sageserpent-open / kineticMerge

Merge a heavily refactored codebase and stay sane.
MIT License
12 stars 2 forks source link

Mysterious quiet merge resolution. #22

Closed sageserpent-open closed 10 months ago

sageserpent-open commented 10 months ago

Issue #19 mentions an odd trailing section that git merge-file was able to quietly merge all by itself when Kinetic Merge's core merge algorithm reported a conflict. This is due to the core merging algorithm making a right file that is just the left file with an appended section (when using a 10% match threshold).

sageserpent-open commented 10 months ago

Revisiting the example repository in issue #19 and debugging through the merge, we see a single edit where the entire left file is substituted for the first base-right matching section. This is followed by a left-deletion / right-edit conflict where the appended part is added on the right.

The CodeMotionAnalysis instance that drives the merge has 4 sections in the base, 1 section on the left and 3 sections on the right. There are 2 matches, both are between base and right sections - the second match being the one that causes the conflict.

It looks like the base-right matches may have suppressed shorter all-sides matches...

sageserpent-open commented 10 months ago

Dumping the contents of the Sources from the three sides via the debugger and making a convetional three-way diff (ironic, that) confirms the above hypothesis.

Given the a larger pairwise match can suppress a shorter all-sides match, this seems to be a fair outcome according to the specifications of CodeMotionAnalysis and merge.

Knocking the matching threshold down to 5% yields the following conflict, which is the other way around with the left file supplying the appended part:

    packageExecutable := {
      val packagingVersion = (ThisBuild / version).value

      println(s"Packaging executable with version: $packagingVersion")

      val localArtifactCoordinates =
        s"${organization.value}:${name.value}_${scalaBinaryVersion.value}:$packagingVersion"

      val executablePath = s"${target.value}${Path.sep}${name.value}"

      s"cs bootstrap --verbose --bat=true --scala-version ${scalaBinaryVersion.value} -f $localArtifactCoordinates -o $executablePath" !

      name.value
    },
    packageExecutable := (packageExecutable dependsOn publishLocal).value,
    libraryDependencies += "com.github.scopt" %% "scopt" % "4.1.0",
    libraryDependencies += "org.typelevel" %% "cats-collections-core" % "0.9.6",
    libraryDependencies += "org.typelevel" %% "cats-core"   % "2.10.0",
    libraryDependencies += "org.typelevel" %% "cats-effect" % "3.5.2",
    libraryDependencies ++= Seq(
      "dev.optics" %% "monocle-core"  % "3.2.0",
      "dev.optics" %% "monocle-macro" % "3.2.0"
    ),
    libraryDependencies += "org.scala-lang.modules" %% "scala-parser-combinators" % "2.3.0",
    libraryDependencies += "com.lihaoyi"             %% "os-lib"  % "0.9.1",
    libraryDependencies += "com.lihaoyi"             %% "fansi"   % "0.4.0",
    libraryDependencies += "com.softwaremill.common" %% "tagging" % "2.3.4",
    libraryDependencies += "com.google.guava" % "guava"     % "32.1.2-jre",
    libraryDependencies += "com.sageserpent" %% "americium" % "1.16.0" % Test,
    libraryDependencies += "com.lihaoyi"     %% "pprint"    % "0.8.1"  % Test,
    libraryDependencies += "com.eed3si9n.expecty" %% "expecty" % "0.16.0" % Test,
    libraryDependencies += "net.aichler" % "jupiter-interface" % JupiterKeys.jupiterVersion.value % Test,
    libraryDependencies += "junit" % "junit" % "4.13.2" % Test,
    Test / fork                   := true,
    Test / testForkedParallel     := true,
    Test / javaOptions ++= Seq("-Xms10G", "-Xmx10G")
  )
<<<<<<< destination
,
    shadingVerbose := true,
    shadedJars += (rabinFingerprint / Compile / packageBin).value,
    shadingRules ++= Seq(
      ShadingRule.moveUnder("org.rabinfingerprint", "shaded")
    ),
    validNamespaces ++= Set("com", "org", "shaded"),
    validEntries ++= Set("version.txt", "usage.txt"),
    packageBin := shadedPackageBin.value
  )
  // Just an optional dependency - otherwise Coursier will pick this up. It's
  // enough to allow IntelliJ to both import from SBT and run tests, given that
  // the Rabin fingerprint code is shaded into Kinetic Merge.
  .dependsOn(rabinFingerprint % Optional)

lazy val rabinFingerprint = (project in file("rabinfingerprint")).settings(
  publishMavenStyle                        := false,
  crossPaths                               := false,
  packageBin / publishArtifact             := false,
  packageSrc / publishArtifact             
=======
>>>>>>> THEIRS
sageserpent-open commented 10 months ago

Using git merge --no-commit to do the same merge yields:

    packageExecutable := {
<<<<<<< HEAD
      val packagingVersion = (ThisBuild / version).value

      println(s"Packaging executable with version: $packagingVersion")
=======
      val _ = publishLocal.value
>>>>>>> THEIRS

      val localArtifactCoordinates =
        s"${organization.value}:${name.value}_${scalaBinaryVersion.value}:$packagingVersion"

      val executablePath = s"${target.value}${Path.sep}${name.value}"

      s"cs bootstrap --verbose --bat=true --scala-version ${scalaBinaryVersion.value} -f $localArtifactCoordinates -o $executablePath" !

      name.value
    },
    packageExecutable := (packageExecutable dependsOn publishLocal).value,
    libraryDependencies += "com.github.scopt" %% "scopt" % "4.1.0",
    libraryDependencies += "org.typelevel" %% "cats-collections-core" % "0.9.6",
    libraryDependencies += "org.typelevel" %% "cats-core"   % "2.10.0",
    libraryDependencies += "org.typelevel" %% "cats-effect" % "3.5.2",
    libraryDependencies ++= Seq(
      "dev.optics" %% "monocle-core"  % "3.2.0",
      "dev.optics" %% "monocle-macro" % "3.2.0"
    ),
    libraryDependencies += "org.scala-lang.modules" %% "scala-parser-combinators" % "2.3.0",
    libraryDependencies += "com.lihaoyi"             %% "os-lib"  % "0.9.1",
    libraryDependencies += "com.lihaoyi"             %% "fansi"   % "0.4.0",
    libraryDependencies += "com.softwaremill.common" %% "tagging" % "2.3.4",
    libraryDependencies += "com.google.guava" % "guava"     % "32.1.2-jre",
    libraryDependencies += "com.sageserpent" %% "americium" % "1.16.0" % Test,
    libraryDependencies += "com.lihaoyi"     %% "pprint"    % "0.8.1"  % Test,
    libraryDependencies += "com.eed3si9n.expecty" %% "expecty" % "0.16.0" % Test,
    libraryDependencies += "net.aichler" % "jupiter-interface" % JupiterKeys.jupiterVersion.value % Test,
<<<<<<< HEAD
    Test / fork               := true,
    Test / testForkedParallel := true,
    Test / javaOptions ++= Seq("-Xms10G", "-Xmx10G"),
    shadingVerbose := true,
    shadedJars += (rabinFingerprint / Compile / packageBin).value,
    shadingRules ++= Seq(
      ShadingRule.moveUnder("org.rabinfingerprint", "shaded")
    ),
    validNamespaces ++= Set("com", "org", "shaded"),
    validEntries ++= Set("version.txt", "usage.txt"),
    packageBin := shadedPackageBin.value
  )
  // Just an optional dependency - otherwise Coursier will pick this up. It's
  // enough to allow IntelliJ to both import from SBT and run tests, given that
  // the Rabin fingerprint code is shaded into Kinetic Merge.
  .dependsOn(rabinFingerprint % Optional)

lazy val rabinFingerprint = (project in file("rabinfingerprint")).settings(
  publishMavenStyle                        := false,
  crossPaths                               := false,
  packageBin / publishArtifact             := false,
  packageSrc / publishArtifact             := false,
  packageDoc / publishArtifact             := false,
  libraryDependencies += "com.google.guava" % "guava" % "32.1.2-jre",
  libraryDependencies += "junit"            % "junit" % "4.13.2" % Test
)
=======
    libraryDependencies += "junit" % "junit" % "4.13.2" % Test,
    Test / fork                   := true,
    Test / testForkedParallel     := true,
    Test / javaOptions ++= Seq("-Xms10G", "-Xmx10G")
  )
>>>>>>> THEIRS
sageserpent-open commented 10 months ago

Kinetic Merge has done a better job in that it completely avoided the first conflict reported by Git's merge - that was what issue #19 was all about.

In the absence of the tool actually understanding SBT sources, I think Kinetic Merge has done a reasonable job, picking up on the fine-grained similarities between the two sides of the second conflict reported by Git merge, namely the:

    Test / fork                   := true,
    Test / testForkedParallel     := true,
    Test / javaOptions ++= Seq("-Xms10G", "-Xmx10G")

I think I'm OK for now with the outcome, more in-depth exploration in ticket #21 will highlight if this is fundamentally wrong.

sageserpent-open commented 10 months ago

There is an aside - when git merge-file quietly merges the supposedly conflicted file, it means that simply invoking git merge --abort will not roll back the merge.

Issuing a git status prior to attempting the rollback allows git merge --abort to work correctly.

This seems to be connected with a Git bug mentioned on Stack Overflow and indeed the workaround is mentioned there too.

sageserpent-open commented 10 months ago

Addressed the Git rollback issue; the user is also alerted when the conflicted file is trivially merged.

Complete in Git commit SHA d9cfb86ce9adb518f045049671f4e36b33c36bbf.

sageserpent-open commented 9 months ago

This went out in release 0.1.10, Git commit SHA 64fd5448f2f9e6d36953c86345d069d62d1d3a2f.