Correct five inaccurate claims in the physical_key comments

Review of c3a9243 found five comments whose stated mechanism does not
match the code:

- `install_paths` doc claimed retaining the remote `s3://` key is what
  lets a push skip the re-upload. It is not — `matches_content` compares
  logical_key/hash/size only, so dedup is identical with `file://` keys.
  Also dropped the "installed rows keep the remote `s3://` key" framing:
  this function copies rows verbatim, and the installed manifest it is
  handed carries `file://` for locally committed paths. Recorded the live
  consequence instead — the caching call parses `physical_key` as an
  `S3Uri`, so an uncached `file://` row errors rather than being fetched.

- `push_package_impl` claimed a later pull or reset clears the stale
  `file://` keys. Neither can: both short-circuit when `latest` is the
  hash just pushed. They clear only when another client publishes.

- `tag_timestamp`'s collision note read as if the second-pushed revision
  loses its history entry; it is the first.

- `upload_row`'s doc credited the `file://` scheme check with keeping
  local keys out of published manifests. That comes from the
  `remote_url` rewrite plus the content-match branch; the check guards
  the input, and for `s3://` keys `to_file_path` would fail anyway.

Comments only; no behavior change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: fiskus

quilt-rs/src/flow/install_paths.rs

@@ -72,23 +72,19 @@ async fn stream_remote_with_installed_rows(
 
 /// Installs paths to already existing manifest (provided as an argument to this function).
 ///
-/// Installed rows keep the remote `s3://` `physical_key`; they are **not**
+/// Rows go into the installed manifest **verbatim** — `physical_key` is never
 /// rewritten to the `file://` object-store location, despite the `place` value
-/// computed in the loop below (logged, never consumed) and the plan sketch's
-/// "replace entry's physical key" step (never implemented).
+/// computed below (logged, never consumed) and the plan sketch's "replace
+/// entry's physical key" step (never implemented). Nothing needs the rewrite:
+/// [`matches_content`](crate::manifest::ManifestRow::matches_content) ignores
+/// `physical_key`, so a later push dedups by content whatever the scheme.
 ///
-/// That is deliberate: `physical_key` names where the canonical bytes live,
-/// and it is what lets a later push skip the re-upload —
-/// [`crate::manifest::ManifestRow::matches_content`] ignores `physical_key`,
-/// so `push`'s `use_existing_row_or_upload` copies the remote key across.
-///
-/// So a `file://` key means "these bytes were committed locally" — written by
-/// [`commit`](fn@super::commit) and [`create`](fn@super::create) — but it is
-/// **not** a local-vs-remote origin marker: a successful push does not rewrite
-/// the installed manifest, so `file://` rows outlive their push and clear only
-/// when a later pull or reset refreshes the installed copy from the manifest
-/// cache. That drift is inert — `top_hash` excludes `physical_key` (see
-/// `manifest::top_hasher`), so both copies are the same revision.
+/// A row's scheme is therefore just its source manifest's, not an origin
+/// marker. `file://` means the bytes were committed locally
+/// ([`commit`](fn@super::commit) / [`create`](fn@super::create)) and outlives a
+/// push, which never rewrites the installed manifest. Watch out — the caching
+/// call below parses `physical_key` as an `S3Uri`, so a `file://` row whose
+/// object is missing from the cache errors instead of being fetched.
 // TODO: `working_dir` is in `paths` already, and we pass namespace anyway
 //       so we can remove working_dir from the arguments
 #[allow(clippy::too_many_arguments)]
@@ -166,10 +162,9 @@ pub async fn install_paths(
             debug!("✔️ Cached object: {}", object_dest.display());
         }
 
-        // Diagnostic only: `place` is the `file://` URL the row *would* carry
-        // if installed rows were rewritten to the object store. They are not
-        // (see above), so this is logged and discarded — the error arm still
-        // asserts `object_dest` is a valid absolute path.
+        // Diagnostic only: the `file://` URL the row *would* carry if rows were
+        // rewritten to the object store (they are not — see above). Logged and
+        // discarded; the error arm still asserts `object_dest` is absolute.
         let place = Url::from_file_path(&object_dest)
             .map_err(|()| Error::InstallPath(InstallPathError::Install(object_dest.clone())))?
             .to_string();
@@ -178,7 +173,7 @@ pub async fn install_paths(
             object_dest.display(),
             place
         );
-        // The row goes in unchanged — remote `physical_key` and all.
+        // The row goes in unchanged — `physical_key` and all.
         entries.insert(row.logical_key.clone(), row.clone());
 
         let working_dest = working_dir.join(&row.logical_key);

quilt-rs/src/flow/push.rs

@@ -233,7 +233,9 @@ pub(crate) async fn push_package_impl(
     // path never copies it over the installed one (`copy_cached_to_installed`
     // below runs only on the hash-mismatch error). So a locally committed row
     // keeps its `file://` `physical_key` in `.quilt/installed/` after a
-    // successful push, until a later pull or reset refreshes it from this cache.
+    // successful push, and stays that way: pull and reset both short-circuit
+    // once `latest` is the hash we just pushed, so those keys only go away when
+    // *another* client publishes a newer revision.
     //
     // Safe because `top_hash` excludes `physical_key` (see
     // `manifest::top_hasher`) — the two copies are the same revision, which is

quilt-rs/src/io/manifest.rs

@@ -105,9 +105,9 @@ pub async fn tag_timestamp(
     // (e.g., try five times with exponential backoff, then Error)
     // NB: the key has one-second resolution and holds the *commit* time, so two
     // commits of the same namespace made within one second collide even if
-    // pushed hours apart. `upload_tag` overwrites, so whichever is pushed second
-    // silently replaces the other's tag — that revision keeps its manifest but
-    // loses its history entry.
+    // pushed hours apart. `upload_tag` overwrites, so the second push silently
+    // takes over the tag and the *first* revision is left with a manifest but no
+    // history entry.
     let tag_timestamp = TagUri::timestamp(manifest_uri, Seconds(timestamp.timestamp()));
     upload_tag(remote, manifest_uri, tag_timestamp).await
 }
@@ -193,14 +193,16 @@ pub async fn resolve_manifest_uri(
 /// though it should be the same as already calclulated during commit.
 /// Response with the new `ManifestRow` with `physical_key` pointing to the place it was uploaded to.
 ///
-/// The `file://` scheme check below is load-bearing, not defensive: together
-/// with `push::use_existing_row_or_upload`'s other branch (which copies the
-/// remote row's `physical_key` across on a content match) it is what keeps a
-/// local key out of a published manifest — every row of an uploaded manifest
-/// takes one of those two paths. It also closes the reverse leak: a row already
-/// keyed `s3://` but not matching the destination's row (a package re-pointed
-/// at another bucket, say) hard-errors instead of publishing a manifest that
-/// addresses another bucket's objects.
+/// A published row's `physical_key` is always remote: either rewritten below to
+/// the `remote_url` this upload returns, or copied from the remote row by
+/// `push::use_existing_row_or_upload`'s content-match branch. That pair — not
+/// the scheme check — is what keeps a local key out of a published manifest.
+///
+/// The check guards the *input* instead: a row that is not local has no business
+/// being uploaded, so a package re-pointed at another bucket hard-errors rather
+/// than republishing another bucket's objects. (An `s3://` key would fail
+/// `to_file_path` anyway; the check's own catch is host-less schemes like
+/// `foo:///abs/path`.)
 pub async fn upload_row(
     remote: &impl Remote,
     host_config: &HostConfig,