Put the commit first in the build stamp, and compare only that

build.gradle.kts

@@ -61,18 +61,26 @@ abstract class GitLatestTagValueSource : ValueSource<String, ValueSourceParamete
 }
 
 /**
- * Which build this is, in one string: where it was built and from what commit.
+ * Which build this is, in one string: what commit it came from and where it was built.
  *
  * The version code is the commit count on origin/master, so every branch build carries master's
  * number: a build flashed from a feature branch and one flashed from master both report "v2.0
  * (3052)" and cannot be told apart on the device. That is not hypothetical — it cost a real
  * investigation to establish which of the two was installed.
  *
- * Two shapes, because "where did this binary come from" has two different answers:
+ * **The commit always comes first, and what follows always says where.** Both readers of this string
+ * want the commit and nothing else — the manager compares it against the SHA a release was cut from
+ * to answer "am I running this build", and the status page sets it in a type the rest is not given
+ * — and for both it is now simply the head of the string. No rule has to work out which part is
+ * which, so none can get it wrong when a repository or a machine turns out to have a hyphen in its
+ * name.
  *
- * *On GitHub Actions*, `JingMatrix-Vector-93d66473` — the repository the code came from, then the
- * commit. Forks build the same code from the same commits, so the hash alone identifies a
- * *revision* and not a *build*, and a fork's artifact was indistinguishable from ours.
+ * The separator says which kind of "where" follows, because they mean opposite things about whether
+ * the commit describes the binary:
+ *
+ * *On GitHub Actions*, `93d66473-JingMatrix-Vector` — the commit, then the repository the code came
+ * from. Forks build the same code from the same commits, so the hash alone identifies a *revision*
+ * and not a *build*, and a fork's artifact was indistinguishable from ours.
  *
  * Both halves are the *head* ones, and neither is read from the environment GitHub sets by default,
  * because on a pull request both defaults name something other than the code that was built:
@@ -91,11 +99,17 @@ abstract class GitLatestTagValueSource : ValueSource<String, ValueSourceParamete
  * when there is not — outside a pull request the two agree anyway. Absent either, this falls back to
  * what the environment says and to `HEAD`, which covers a workflow too old to set them.
  *
- * *Locally*, the bare `93d66473`, or `93d66473-thinkpad` when the tree has uncommitted changes.
- * That build corresponds to no commit at all, so naming the commit alone would name something the
- * binary does not match — and whoever is looking at it is far better served by the machine it was
- * built on than by the word "dirty", since a device that has a hand-built framework on it usually
- * has exactly one candidate author.
+ * *Locally*, the bare `93d66473`, or `93d66473+thinkpad` when the tree has uncommitted changes. That
+ * build corresponds to no commit at all, so naming the commit alone would name something the binary
+ * does not match — and whoever is looking at it is far better served by the machine it was built on
+ * than by the word "dirty", since a device that has a hand-built framework on it usually has exactly
+ * one candidate author.
+ *
+ * `+` rather than `-` for that one, in semver's sense of build metadata: after a `-` is a repository
+ * that holds this exact commit, and after a `+` is a change that is in no repository at all. That is
+ * the one distinction a reader of the stamp cannot afford to lose — a modified build matches no
+ * release, and read as a CI build it would claim to be the release it was merely started from.
+ * Neither character can occur inside a host name or an `owner/repo`, so the two shapes stay apart.
  *
  * The dirty check is deliberately not applied on CI. The workflow's own "Write key" step appends
  * the signing credentials to the tracked `gradle.properties` before Gradle starts, so every master
@@ -165,7 +179,7 @@ abstract class GitCommitHashValueSource : ValueSource<String, GitCommitHashValue
  val repository = parameters.buildRepository.getOrElse("")
  if (repository.isBlank()) {
  val dirty = capture("git", "status", "--porcelain", "--untracked-files=no") != null
- return if (dirty) "$head-${hostname()}" else head
+ return if (dirty) "$head+${hostname()}" else head
  }
 
  // The pushed commit is an ancestor of the merge that was checked out, so it is in the
@@ -174,7 +188,7 @@ abstract class GitCommitHashValueSource : ValueSource<String, GitCommitHashValue
  val pushed = parameters.buildCommit.getOrElse("").takeIf { it.isNotBlank() }
  val short =
  pushed?.let { capture("git", "rev-parse", "--short", it) ?: it.take(head.length) } ?: head
- return repository.replace('/', '-') + "-" + short
+ return short + "-" + repository.replace('/', '-')
  }
 }
 

manager/README.md

@@ -86,8 +86,11 @@ asserting the old meaning until someone notices, which can be a long time.
 ```
 
 The version code is `git rev-list --count refs/remotes/origin/master`, so a branch build and a
-master build can share one; `module.prop` carries the commit, marked `-dirty` when the tree was not
-clean, which is often the only way to tell two builds apart on a device.
+master build can share one; `module.prop` and the status page carry a build stamp, which is often
+the only way to tell two builds apart on a device. It names where the build came from as well as
+what it was built from. The commit leads and what follows says where: `93d66473-JingMatrix-Vector`
+for a CI build, the bare `93d66473` for a local one, and `93d66473+thinkpad` — the machine that made
+it — when the tree was not clean.
 
 Debug builds add a second launcher activity — a demo mode with scripted device states, in
 `src/debug` and absent from release builds, so it cannot be used to make a release report a healthy

manager/src/main/kotlin/org/matrix/vector/manager/data/model/BuildStamp.kt

@@ -0,0 +1,74 @@
+package org.matrix.vector.manager.data.model
+
+/**
+ * A build stamp, taken apart: which commit a build came from, and where it was built.
+ *
+ * The framework and the manager both report one — `BuildConfig.VERSION_HASH`, and
+ * `getFrameworkCommit()` across the binder — because the version code cannot tell two builds apart:
+ * it is the commit count on origin/master, so every branch build at the same depth wears the same
+ * number as the official build it was never made from.
+ *
+ * Only the commit is taken out. Where a build was made is for a person to read, and it is exactly
+ * the rest of the string — a caller that wants it takes what follows the commit, separator and all.
+ *
+ * @property commit the commit the build was made from, abbreviated as git abbreviated it, or null
+ * when the stamp names none — "unknown" from a build made outside a git checkout, and anything
+ * else this does not recognise. That is a third answer, distinct from "these differ".
+ * @property modified whether the tree had uncommitted changes. Such a build was made from no commit
+ * at all: [commit] names the one it departed from, not one the binary corresponds to.
+ */
+data class BuildStamp(val commit: String?, val modified: Boolean) {
+
+ /**
+ * Whether this build and [other] were made from the same commit.
+ *
+ * A modified tree matches nothing, including itself. Otherwise it is a prefix test in either
+ * direction, because the two sides need not be abbreviated to the same length: a stamp carries
+ * git's short form and a GitHub release carries the full SHA.
+ *
+ * **Where the build was made is deliberately not compared.** A fork building the same commit
+ * builds the same code; the repository is in the stamp to identify a *binary* to someone
+ * reading a bug report, which is a different question from "is this the build that release
+ * published".
+ */
+ fun isCommit(other: String?): Boolean {
+ if (modified || other == null) return false
+ val ours = commit ?: return false
+ return other.startsWith(ours) || ours.startsWith(other)
+ }
+}
+
+/**
+ * Reads a build stamp.
+ *
+ * The commit leads, always, and the separator says what follows it — `-` a repository that holds
+ * this exact commit, `+` a machine that holds changes no repository does. So the commit is the head
+ * and there is nothing to guess: neither character can occur inside a host name or an `owner/repo`,
+ * and a hyphen inside either is harmless because everything after the first one is the origin.
+ *
+ * - `93d66473` — a local build of a clean tree.
+ * - `93d66473-JingMatrix-Vector` — CI, from `JingMatrix/Vector`.
+ * - `93d66473+thinkpad` — a local build with uncommitted changes, named by the machine that made it.
+ *
+ * A stamp that does not start with a commit yields a null one, which everywhere it is asked means
+ * "I cannot tell" rather than "these differ". That covers the shape shipped between #809 and this
+ * change, `JingMatrix-Vector-93d66473`, which is what the canaries already on people's devices
+ * carry: they are shown as they were recorded and claimed to be neither installed nor divergent,
+ * which is the truthful answer for a build whose stamp this cannot read.
+ */
+fun buildStamp(reported: String): BuildStamp {
+ val stamp = reported.trim()
+ val head = stamp.takeWhile { it != '-' && it != '+' }
+ if (!head.isAbbreviatedSha()) return BuildStamp(null, modified = false)
+ return BuildStamp(commit = head, modified = stamp.getOrNull(head.length) == '+')
+}
+
+/**
+ * Whether this could be a commit as git abbreviates one.
+ *
+ * Seven is git's own floor for an abbreviation and forty is a full SHA. Lower case only, which git
+ * emits and which is what keeps the word "unknown" — a build made where git could not be asked —
+ * from being mistaken for one.
+ */
+private fun String.isAbbreviatedSha(): Boolean =
+ length in 7..40 && all { it.isDigit() || it in 'a'..'f' }

manager/src/main/kotlin/org/matrix/vector/manager/data/repository/FrameworkUpdateRepository.kt

@@ -5,6 +5,7 @@ import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.flow.asStateFlow
 import org.matrix.vector.manager.data.github.FrameworkRelease
 import org.matrix.vector.manager.data.github.GitHubRepository
+import org.matrix.vector.manager.data.model.buildStamp
 
 /**
  * Whether a newer build of the framework exists, and which one this reader may be offered.
@@ -70,7 +71,12 @@ class FrameworkUpdateRepository(private val github: GitHubRepository) {
  */
 data class FrameworkUpdateState(
  val installedVersionCode: Long = 0,
- /** The commit the running daemon was built from, when it recorded one. */
+ /**
+ * The build stamp the running daemon reports, when it recorded one.
+ *
+ * Not a bare hash: it names where the build came from as well as what commit it was made from,
+ * in one of the shapes [buildStamp] reads. Nothing here compares it as a string.
+ */
  val installedCommit: String? = null,
  val available: FrameworkRelease? = null,
  /** Every release on this channel, newest first — including ones older than the installed one. */
@@ -93,12 +99,16 @@ enum class ReleaseDirection {
  * Only askable when both sides recorded a commit: the canaries carry a SHA, a hand-made release
  * carries a branch name, and a build made before this existed carries nothing. "I cannot tell" is a
  * third answer and is reported as false rather than as divergence.
+ *
+ * What the framework reports is a *build stamp*, not a bare hash, so the commit is read out of it
+ * before anything is compared. Comparing the whole stamp is what #809 left behind: a CI stamp
+ * carries the repository as well, no release SHA matches that, and so every canary reader was told
+ * they were running "same number, other build" against the very release they had flashed, with no
+ * row anywhere marked as installed.
  */
 fun FrameworkUpdateState.divergesFrom(release: FrameworkRelease?): Boolean {
  if (release == null || release.versionCode != installedVersionCode) return false
- val mine = installedCommit ?: return false
- val theirs = release.commit ?: return false
- // A dirty build matches nothing by definition — it was not built from any commit.
- if (mine.endsWith("-dirty")) return true
- return !theirs.startsWith(mine)
+ val mine = buildStamp(installedCommit ?: return false)
+ if (mine.commit == null || release.commit == null) return false
+ return !mine.isCommit(release.commit)
 }

manager/src/main/kotlin/org/matrix/vector/manager/ui/screens/home/HomeViewModel.kt

@@ -47,13 +47,14 @@ data class FrameworkStatus(
  val sepolicyLoaded: Boolean = false,
  val systemServerInjected: Boolean = false,
  /**
- * Which build the running framework is: where it came from and from what commit.
+ * Which build the running framework is: what commit it came from and where it was built.
  *
  * The version code is a commit count, so it cannot tell a branch build from the official build
  * of the same depth — and the framework and the manager are flashed separately, so they are
  * not always the same build. Naming both is the difference between a bug report that can be
- * placed and one that cannot. A CI build reads `JingMatrix-Vector-93d66473`, a clean local one
- * the bare hash, and a local build from a modified tree adds the machine that made it.
+ * placed and one that cannot. The commit leads: a CI build reads `93d66473-JingMatrix-Vector`,
+ * a clean local one the bare hash, and a local build from a modified tree marks the machine
+ * that made it with a `+`. Taken apart by `buildStamp`; nothing compares it as a string.
  */
  val commit: String? = null,
 ) {