Allow git push --dry-run to store metadata

Author: glandium

README.md

@@ -196,9 +196,11 @@ preference with one of the following values:
 - `always`
 - `never`
 - `phase`
+- `force`
 
 `phase` is the default described above. `always` and `never` are
-self-explanatory.
+self-explanatory. `force` has the same meaning as `always`, but also
+forces `git push --dry-run` to store metadata.
 
 Cinnabar clone:
 ---------------

src/main.rs

@@ -4478,6 +4478,39 @@ fn check_graft_refs() {
     }
 }
 
+#[derive(PartialEq, Eq)]
+enum RecordMetadata {
+    Never,
+    Phase,
+    Always,
+    // Like Always, but also applies to dry-run
+    Force,
+}
+
+impl TryFrom<&OsStr> for RecordMetadata {
+    type Error = String;
+
+    fn try_from(value: &OsStr) -> Result<RecordMetadata, String> {
+        value
+            .to_str()
+            .and_then(|s| {
+                Some(match s {
+                    "never" => RecordMetadata::Never,
+                    "phase" => RecordMetadata::Phase,
+                    "always" => RecordMetadata::Always,
+                    "force" => RecordMetadata::Force,
+                    _ => return None,
+                })
+            })
+            .ok_or_else(|| {
+                format!(
+                    "`{}` is not one of `never`, `phase`, `always` or `force`",
+                    value.as_bytes().as_bstr()
+                )
+            })
+    }
+}
+
 fn remote_helper_push(
     store: &mut Store,
     conn: &mut dyn HgRepo,
@@ -4530,6 +4563,11 @@ fn remote_helper_push(
         }
     });
 
+    let data = get_config_remote("data", remote)
+        .filter(|d| !d.is_empty())
+        .as_deref()
+        .map_or(Ok(RecordMetadata::Phase), RecordMetadata::try_from)?;
+
     let mut pushed = ChangesetHeads::new();
     let result = (|| {
         let push_commits = push_refs.iter().filter_map(|(c, _, _)| *c).collect_vec();
@@ -4692,7 +4730,7 @@ fn remote_helper_push(
         }
 
         let mut result = None;
-        if !push_commits.is_empty() && !dry_run {
+        if !push_commits.is_empty() && (!dry_run || data == RecordMetadata::Force) {
             conn.require_capability(b"unbundle");
 
             let b2caps = conn
@@ -4718,6 +4756,7 @@ fn remote_helper_push(
                     .unwrap_or(1);
                 (BundleSpec::V2None, version)
             };
+            // TODO: Ideally, for dry-run, we wouldn't even create a temporary file.
             let tempfile = tempfile::Builder::new()
                 .prefix("hg-bundle-")
                 .suffix(".hg")
@@ -4734,40 +4773,43 @@ fn remote_helper_push(
                 version == 2,
             )?;
             drop(file);
-            let file = File::open(path).unwrap();
-            let empty_heads = [HgChangesetId::NULL];
-            let heads = if force {
-                None
-            } else if no_topological_heads {
-                Some(&empty_heads[..])
-            } else {
-                Some(&info.topological_heads[..])
-            };
-            let response = conn.unbundle(heads, file);
-            match response {
-                UnbundleResponse::Bundlev2(data) => {
-                    let mut bundle = BundleReader::new(data).unwrap();
-                    while let Some(part) = bundle.next_part().unwrap() {
-                        match part.part_type.as_bytes() {
-                            b"reply:changegroup" => {
-                                // TODO: should check in-reply-to param.
-                                let response = part.get_param("return").unwrap();
-                                result = u32::from_str(response).ok();
-                            }
-                            b"error:abort" => {
-                                let mut message = part.get_param("message").unwrap().to_string();
-                                if let Some(hint) = part.get_param("hint") {
-                                    message.push_str("\n\n");
-                                    message.push_str(hint);
+            if !dry_run {
+                let file = File::open(path).unwrap();
+                let empty_heads = [HgChangesetId::NULL];
+                let heads = if force {
+                    None
+                } else if no_topological_heads {
+                    Some(&empty_heads[..])
+                } else {
+                    Some(&info.topological_heads[..])
+                };
+                let response = conn.unbundle(heads, file);
+                match response {
+                    UnbundleResponse::Bundlev2(data) => {
+                        let mut bundle = BundleReader::new(data).unwrap();
+                        while let Some(part) = bundle.next_part().unwrap() {
+                            match part.part_type.as_bytes() {
+                                b"reply:changegroup" => {
+                                    // TODO: should check in-reply-to param.
+                                    let response = part.get_param("return").unwrap();
+                                    result = u32::from_str(response).ok();
+                                }
+                                b"error:abort" => {
+                                    let mut message =
+                                        part.get_param("message").unwrap().to_string();
+                                    if let Some(hint) = part.get_param("hint") {
+                                        message.push_str("\n\n");
+                                        message.push_str(hint);
+                                    }
+                                    error!(target: "root", "{}", message);
                                 }
-                                error!(target: "root", "{}", message);
+                                _ => {}
                             }
-                            _ => {}
                         }
                     }
-                }
-                UnbundleResponse::Raw(response) => {
-                    result = u32::from_bytes(&response).ok();
+                    UnbundleResponse::Raw(response) => {
+                        result = u32::from_bytes(&response).ok();
+                    }
                 }
             }
         }
@@ -4851,76 +4893,60 @@ fn remote_helper_push(
         stdout.flush().unwrap();
     }
 
-    let data = get_config_remote("data", remote);
-    let data = data
-        .as_deref()
-        .and_then(|d| (!d.is_empty()).then_some(d))
-        .unwrap_or_else(|| OsStr::new("phase"));
-    let valid = [
-        OsStr::new("never"),
-        OsStr::new("phase"),
-        OsStr::new("always"),
-    ];
-    if !valid.contains(&data) {
-        die!(
-            "`{}` is not one of `never`, `phase` or `always`",
-            data.as_bytes().as_bstr()
-        );
-    }
-    let rollback = if status.is_empty() || pushed.is_empty() || dry_run {
-        true
-    } else {
-        match data.to_str().unwrap() {
-            "always" => false,
-            "never" => true,
-            "phase" => {
-                let phases = conn.phases();
-                let phases = ByteSlice::lines(&*phases)
-                    .filter_map(|l| {
-                        l.splitn_exact(b'\t')
-                            .map(|[k, v]| (k.as_bstr(), v.as_bstr()))
-                    })
-                    .collect::<HashMap<_, _>>();
-                let drafts = (!phases.contains_key(b"publishing".as_bstr()))
-                    .then(|| {
-                        phases
-                            .into_iter()
-                            .filter_map(|(phase, is_draft)| {
-                                u32::from_bytes(is_draft).ok().and_then(|is_draft| {
-                                    (is_draft > 0).then(|| HgChangesetId::from_bytes(phase))
+    let rollback =
+        if status.is_empty() || pushed.is_empty() || (dry_run && data != RecordMetadata::Force) {
+            true
+        } else {
+            match data {
+                RecordMetadata::Force | RecordMetadata::Always => false,
+                RecordMetadata::Never => true,
+                RecordMetadata::Phase => {
+                    let phases = conn.phases();
+                    let phases = ByteSlice::lines(&*phases)
+                        .filter_map(|l| {
+                            l.splitn_exact(b'\t')
+                                .map(|[k, v]| (k.as_bstr(), v.as_bstr()))
+                        })
+                        .collect::<HashMap<_, _>>();
+                    let drafts = (!phases.contains_key(b"publishing".as_bstr()))
+                        .then(|| {
+                            phases
+                                .into_iter()
+                                .filter_map(|(phase, is_draft)| {
+                                    u32::from_bytes(is_draft).ok().and_then(|is_draft| {
+                                        (is_draft > 0).then(|| HgChangesetId::from_bytes(phase))
+                                    })
                                 })
-                            })
-                            .collect::<Result<Vec<_>, _>>()
-                    })
-                    .transpose()
-                    .unwrap()
-                    .unwrap_or_default();
-                if drafts.is_empty() {
-                    false
-                } else {
-                    // Theoretically, we could have commits with no
-                    // metadata that the remote declares are public, while
-                    // the rest of our push is in a draft state. That is
-                    // however so unlikely that it's not worth the effort
-                    // to support partial metadata storage.
-                    !reachable_subset(
-                        pushed
-                            .heads()
-                            .copied()
-                            .filter_map(|h| h.to_git(store))
-                            .map(Into::into),
-                        drafts
-                            .iter()
-                            .copied()
-                            .filter_map(|h| h.to_git(store))
-                            .map(Into::into),
-                    )
-                    .is_empty()
+                                .collect::<Result<Vec<_>, _>>()
+                        })
+                        .transpose()
+                        .unwrap()
+                        .unwrap_or_default();
+                    if drafts.is_empty() {
+                        false
+                    } else {
+                        // Theoretically, we could have commits with no
+                        // metadata that the remote declares are public, while
+                        // the rest of our push is in a draft state. That is
+                        // however so unlikely that it's not worth the effort
+                        // to support partial metadata storage.
+                        !reachable_subset(
+                            pushed
+                                .heads()
+                                .copied()
+                                .filter_map(|h| h.to_git(store))
+                                .map(Into::into),
+                            drafts
+                                .iter()
+                                .copied()
+                                .filter_map(|h| h.to_git(store))
+                                .map(Into::into),
+                        )
+                        .is_empty()
+                    }
                 }
             }
-            _ => unreachable!(),
-        }
-    };
+        };
     if rollback {
         unsafe {
             do_cleanup(1);

tests/push.t

@@ -364,6 +364,32 @@ Server is now non-publishing, so metadata is unchanged.
   WARNING Pushing a new root
   To hg::.*/push.t/xyz (re)
    + 687e015...7ca6a3c 7ca6a3c32ec0dbcbcd155b2be6e2f4505012c273 -> branches/default/tip (forced update)
+
+Whatever happens, --dry-run doesn't store metadata
+
+  $ git -C xyz-git cinnabar rollback --candidates
+  a2341d430e5acddf9481eabcad901fda12d023d3 (current)
+  8b8194eefb69ec89edc35dafb965311fe48c49d0
+  2836e453f32b1ecccd3acca412f75b07c88176bf
+
+There is an extra mode for cinnabar.data that forces --dry-run to store metadata
+
+  $ git -c cinnabar.data=force -C xyz-git push -f --dry-run origin 7ca6a3c32ec0dbcbcd155b2be6e2f4505012c273:refs/heads/branches/default/tip
+  \r (no-eol) (esc)
+  WARNING Pushing a new root
+  To hg::.*/push.t/xyz (re)
+   + 687e015...7ca6a3c 7ca6a3c32ec0dbcbcd155b2be6e2f4505012c273 -> branches/default/tip (forced update)
+  $ git -C xyz-git cinnabar rollback --candidates
+  4305cef3fa610b3370f64ce10d2b50693a904278 (current)
+  a2341d430e5acddf9481eabcad901fda12d023d3
+  8b8194eefb69ec89edc35dafb965311fe48c49d0
+  2836e453f32b1ecccd3acca412f75b07c88176bf
+  $ git -C xyz-git cinnabar rollback
+  $ git -C xyz-git cinnabar rollback --candidates
+  a2341d430e5acddf9481eabcad901fda12d023d3 (current)
+  8b8194eefb69ec89edc35dafb965311fe48c49d0
+  2836e453f32b1ecccd3acca412f75b07c88176bf
+
   $ git -c cinnabar.data=always -C xyz-git push -f origin 7ca6a3c32ec0dbcbcd155b2be6e2f4505012c273:refs/heads/branches/default/tip
   \r (no-eol) (esc)
   WARNING Pushing a new root