Document what physical_key schemes and timestamp tags actually mean

quilt-rs/src/flow/install_paths.rs

@@ -71,7 +71,20 @@ async fn stream_remote_with_installed_rows(
 }
 
 /// Installs paths to already existing manifest (provided as an argument to this function).
-/// It also modifies manifest, because installed paths have `place` pointing to `file://location`
+///
+/// Rows go into the installed manifest **verbatim** — `physical_key` is never
+/// rewritten to the `file://` object-store location, despite the `place` value
+/// 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.
+///
+/// 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)]
@@ -114,7 +127,7 @@ pub async fn install_paths(
     // for each path in entries_paths:
     //   get entry from installed manifest
     //   cache the entry into identity cache (if not there)
-    //   replace entry's physical key in the manifest with the cached physical key
+    //   (the sketch rewrote the physical key here; the code does not — see above)
     //
     // write the adjusted manifest into the installed manifest path
     // copy the selected paths into the working folder
@@ -149,6 +162,9 @@ pub async fn install_paths(
             debug!("✔️ Cached object: {}", object_dest.display());
         }
 
+        // 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();
@@ -157,6 +173,7 @@ pub async fn install_paths(
             object_dest.display(),
             place
         );
+        // 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

@@ -229,6 +229,18 @@ pub(crate) async fn push_package_impl(
         )
         .await,
     );
+    // The uploaded manifest goes into the *cached* manifests dir; the success
+    // 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, 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
+    // also why the `new_manifest_uri.hash != commit.hash` guard below does not
+    // fire on a normal push.
     let dest_dir = paths.cached_manifests_dir(&manifest_uri.bucket);
     let (cache_path, top_hash) =
         build_manifest_from_rows_stream(storage, dest_dir, local_manifest.header.clone(), stream)

quilt-rs/src/io/manifest.rs

@@ -81,6 +81,17 @@ pub async fn upload_manifest(
 
 /// Upload file containing hash of the manifest
 /// "tagged" by timestamp.
+///
+/// This is the only place a revision's publish time reaches the remote: the
+/// manifest format carries no timestamps (neither `ManifestHeader` nor
+/// `ManifestRow` has a time field), so `.quilt/named_packages/<ns>/<epoch>` is
+/// the registry's revision history.
+///
+/// It is **write-only from this client**: reading those times back means
+/// enumerating the tag objects, and [`Remote`] has no list operation. So
+/// nothing here can answer "when was revision X published" or "what revisions
+/// exist". The value written is the *local* `CommitState::timestamp` — when
+/// the commit was made on the pushing machine, not when the upload completed.
 pub async fn tag_timestamp(
     remote: &impl Remote,
     manifest_uri: &ManifestUri,
@@ -92,6 +103,11 @@ pub async fn tag_timestamp(
     // create it with the value of {self.commit.hash}
     // TODO: Otherwise try again with the current timestamp as the tag
     // (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 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
 }
@@ -176,6 +192,17 @@ pub async fn resolve_manifest_uri(
 /// After uploading we get new hash,
 /// 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.
+///
+/// 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,