config: add ConfigExt::must_get

Summary:
must_get is like get_nonempty_opt but returns an error if the value is unset or
empty. This makes it easy to propagate an error if the desired config is not
set.

I also updated a few spots to use must_get and got rid of a superfluous error type.

Reviewed By: quark-zju

Differential Revision: D44857516

fbshipit-source-id: cdecd7a68d4e3d8248cae977f4cdf7b553b6af74

eden/scm/edenscm/error.py

@@ -28,6 +28,7 @@ from . import pycompat
 
 CertificateError = bindings.error.CertificateError
 CommitLookupError = bindings.error.CommitLookupError
+ConfigError = bindings.error.ConfigError
 FetchError = bindings.error.FetchError
 HttpError = bindings.error.HttpError
 IndexedLogError = bindings.error.IndexedLogError
@@ -37,7 +38,6 @@ NeedSlowPathError = bindings.error.NeedSlowPathError
 NonUTF8PathError = bindings.error.NonUTF8Path
 WorkingCopyError = bindings.error.WorkingCopyError
 RepoInitError = bindings.error.RepoInitError
-RevisionstoreError = bindings.error.RevisionstoreError
 UncategorizedNativeError = bindings.error.UncategorizedNativeError
 TlsError = bindings.error.TlsError
 
@@ -182,10 +182,6 @@ class HookAbort(Abort):
         Abort.__init__(self, *args, **kwargs)
 
 
-class ConfigError(Abort):
-    """Exception raised when parsing config files"""
-
-
 class UpdateAbort(Abort):
     """Raised when an update is aborted for destination issue"""
 

eden/scm/edenscm/merge.py

@@ -344,8 +344,9 @@ class mergestate(object):
             and self._readmergedriver != configmergedriver
         ):
             raise error.ConfigError(
-                _("merge driver changed since merge started"),
-                hint=_("revert merge driver change or abort merge"),
+                _("merge driver changed since merge started")
+                + "\n"
+                + _("revert merge driver change or abort merge")
             )
 
         return configmergedriver

eden/scm/edenscm/scmutil.py

@@ -278,9 +278,9 @@ def callcatch(ui, req, func):
         ui.warn(_("(this usually happens after hard reboot or system crash)\n"))
         ui.warn(_("(try '@prog@ doctor' to attempt to fix it)\n"))
     except (
+        error.ConfigError,
         error.NonUTF8PathError,
         error.RepoInitError,
-        error.RevisionstoreError,
         error.WorkingCopyError,
     ) as inst:
         ui.warn(_("%s\n") % inst, error=_("abort"))

eden/scm/edenscmnative/bindings/modules/pyerror/Cargo.toml

@@ -5,6 +5,7 @@ edition = "2021"
 
 [dependencies]
 auth = { path = "../../../../lib/auth" }
+configmodel = { path = "../../../../lib/config/model" }
 cpython_ext = { path = "../../../../lib/cpython-ext" }
 cpython = { version = "0.7", default-features = false }
 dag = { path = "../../../../lib/dag" }

eden/scm/edenscmnative/bindings/modules/pyerror/src/lib.rs

@@ -10,6 +10,7 @@ use cpython_ext::error;
 
 py_exception!(error, CertificateError);
 py_exception!(error, CommitLookupError, exc::KeyError);
+py_exception!(error, ConfigError);
 py_exception!(error, FetchError, exc::KeyError);
 py_exception!(error, HttpError);
 py_exception!(error, IndexedLogError);
@@ -19,7 +20,6 @@ py_exception!(error, NeedSlowPathError);
 py_exception!(error, NonUTF8Path);
 py_exception!(error, WorkingCopyError);
 py_exception!(error, RepoInitError);
-py_exception!(error, RevisionstoreError);
 py_exception!(error, UncategorizedNativeError);
 py_exception!(error, TlsError);
 
@@ -46,11 +46,7 @@ pub fn init_module(py: Python, package: &str) -> PyResult<PyModule> {
     )?;
     m.add(py, "WorkingCopyError", py.get_type::<WorkingCopyError>())?;
     m.add(py, "RepoInitError", py.get_type::<RepoInitError>())?;
-    m.add(
-        py,
-        "RevisionstoreError",
-        py.get_type::<RevisionstoreError>(),
-    )?;
+    m.add(py, "ConfigError", py.get_type::<ConfigError>())?;
     m.add(py, "NonUTF8Path", py.get_type::<NonUTF8Path>())?;
     m.add(py, "TlsError", py.get_type::<TlsError>())?;
 
@@ -143,8 +139,8 @@ fn register_error_handlers() {
                 py,
                 cpython_ext::Str::from(format!("{:?}", e)),
             ))
-        } else if e.is::<revisionstore::Error>() {
-            Some(PyErr::new::<RevisionstoreError, _>(
+        } else if e.is::<configmodel::Error>() {
+            Some(PyErr::new::<ConfigError, _>(
                 py,
                 cpython_ext::Str::from(format!("{:?}", e)),
             ))

eden/scm/lib/config/model/src/config.rs

@@ -15,6 +15,7 @@ use std::sync::Arc;
 use minibytes::Text;
 
 use crate::convert::FromConfigValue;
+use crate::Error;
 use crate::Result;
 
 /// Readable config. This can be used as a trait object.
@@ -111,6 +112,16 @@ pub trait ConfigExt: Config {
     fn get_or_default<T: Default + FromConfigValue>(&self, section: &str, name: &str) -> Result<T> {
         self.get_or(section, name, Default::default)
     }
+
+    /// Get a config item. Convert to type `T`.
+    ///
+    /// If the config item is not set, return Error::NotSet.
+    fn must_get<T: Default + FromConfigValue>(&self, section: &str, name: &str) -> Result<T> {
+        match self.get_nonempty_opt(section, name)? {
+            Some(val) => Ok(val),
+            None => Err(Error::NotSet(section.to_string(), name.to_string())),
+        }
+    }
 }
 
 impl<T: Config> ConfigExt for T {}
@@ -260,4 +271,17 @@ mod tests {
         // Make sure we can pass BTreeMap config to generic func.
         wants_impl(&map);
     }
+
+    #[test]
+    fn test_must_get() {
+        let map: BTreeMap<&str, &str> = vec![("foo.bar", "baz")].into_iter().collect();
+        assert_eq!(
+            map.must_get::<Vec<String>>("foo", "bar").unwrap(),
+            vec!["baz".to_string()]
+        );
+        assert!(matches!(
+            map.must_get::<Vec<String>>("foo", "nope"),
+            Err(Error::NotSet(_, _))
+        ));
+    }
 }

eden/scm/lib/config/model/src/error.rs

@@ -48,6 +48,9 @@ pub enum Error {
     #[error("{0}")]
     General(String),
 
+    #[error("config {0}.{1} is not set")]
+    NotSet(String, String),
+
     #[error("{0}")]
     Other(#[source] anyhow::Error),
 }

eden/scm/lib/hgcommands/src/commands/clone.rs

@@ -435,15 +435,9 @@ pub fn revlog_clone(
 }
 
 fn get_selective_bookmarks(repo: &Repo) -> Result<Vec<String>> {
-    match repo
+    Ok(repo
         .config()
-        .get_opt("remotenames", "selectivepulldefault")?
-    {
-        Some(bms) => Ok(bms),
-        None => {
-            abort!("remotenames.selectivepulldefault config is not set");
-        }
-    }
+        .must_get("remotenames", "selectivepulldefault")?)
 }
 
 #[instrument(skip_all, err, ret)]

eden/scm/lib/revisionstore/src/lfs.rs

@@ -115,7 +115,6 @@ use crate::uniondatastore::UnionHgIdDataStore;
 use crate::util::get_lfs_blobs_path;
 use crate::util::get_lfs_objects_path;
 use crate::util::get_lfs_pointers_path;
-use crate::util::get_str_config;
 
 /// The `LfsPointersStore` holds the mapping between a `HgId` and the content hash (sha256) of the LFS blob.
 struct LfsPointersStore(Store);
@@ -1575,7 +1574,7 @@ impl LfsRemote {
         config: &dyn Config,
         correlator: Option<String>,
     ) -> Result<Self> {
-        let mut url = get_str_config(config, "lfs", "url")?;
+        let mut url: String = config.must_get("lfs", "url")?;
         // A trailing '/' needs to be present so that `Url::join` doesn't remove the reponame
         // present at the end of the config.
         url.push('/');

eden/scm/lib/revisionstore/src/lib.rs

@@ -216,7 +216,6 @@ pub use crate::repack::ToKeys;
 pub use crate::types::ContentHash;
 pub use crate::types::StoreKey;
 pub use crate::uniondatastore::UnionHgIdDataStore;
-pub use crate::util::Error;
 
 #[cfg(any(test, feature = "for-tests"))]
 pub mod testutil;

eden/scm/lib/revisionstore/src/scmstore/builder.rs

@@ -449,14 +449,19 @@ impl<'a> FileStoreBuilder<'a> {
 
 // Return remotefilelog cache path, or None if there is no cache path
 // (e.g. because we have no repo name).
-fn cache_path(
-    config: &dyn Config,
-    suffix: &Option<PathBuf>,
-) -> Result<Option<PathBuf>, crate::Error> {
+fn cache_path(config: &dyn Config, suffix: &Option<PathBuf>) -> Result<Option<PathBuf>> {
     match crate::util::get_cache_path(config, suffix) {
         Ok(p) => Ok(Some(p)),
-        Err(crate::Error::ConfigNotSet(_)) => Ok(None),
-        Err(err) => Err(err),
+        Err(err) => {
+            if matches!(
+                err.downcast_ref::<configmodel::Error>(),
+                Some(configmodel::Error::NotSet(_, _))
+            ) {
+                Ok(None)
+            } else {
+                Err(err)
+            }
+        }
     }
 }
 

eden/scm/lib/revisionstore/src/util.rs

@@ -12,53 +12,30 @@ use std::io::Write;
 use std::path::Path;
 use std::path::PathBuf;
 
+use anyhow::Result;
 use configmodel::Config;
+use configmodel::ConfigExt;
 use edenapi::Stats;
 use hgtime::HgTime;
-use thiserror::Error;
 use tracing::Span;
 use util::path::create_dir;
 use util::path::create_shared_dir;
-use util::path::expand_path;
 
-#[derive(Error, Debug)]
-pub enum Error {
-    #[error("could not find config option {0}")]
-    ConfigNotSet(String),
-
-    #[error(transparent)]
-    Io(#[from] std::io::Error),
+pub fn get_repo_name(config: &dyn Config) -> Result<String> {
+    Ok(config.must_get("remotefilelog", "reponame")?)
 }
 
-pub fn get_str_config(config: &dyn Config, section: &str, name: &str) -> Result<String, Error> {
-    let name = config
-        .get(section, name)
-        .ok_or_else(|| Error::ConfigNotSet(format!("{}.{}", section, name)))?;
-    Ok(name.to_string())
-}
-
-pub fn get_repo_name(config: &dyn Config) -> Result<String, Error> {
-    get_str_config(config, "remotefilelog", "reponame")
-}
-
-fn get_config_cache_path(config: &dyn Config) -> Result<PathBuf, Error> {
+fn get_config_cache_path(config: &dyn Config) -> Result<PathBuf> {
     let reponame = get_repo_name(config)?;
-    let config_path = match config.get_nonempty("remotefilelog", "cachepath") {
-        Some(path) => expand_path(path),
-        None => return Err(Error::ConfigNotSet("remotefilelog.cachepath".into())),
-    };
-    let mut path = PathBuf::new();
-    path.push(&config_path);
+    let mut path: PathBuf = config.must_get("remotefilelog", "cachepath")?;
     create_shared_dir(&path)?;
-    path.push(reponame);
+    path.push(&reponame);
     create_shared_dir(&path)?;
+
     Ok(path)
 }
 
-pub fn get_cache_path(
-    config: &dyn Config,
-    suffix: &Option<impl AsRef<Path>>,
-) -> Result<PathBuf, Error> {
+pub fn get_cache_path(config: &dyn Config, suffix: &Option<impl AsRef<Path>>) -> Result<PathBuf> {
     let mut path = get_config_cache_path(config)?;
 
     if let Some(ref suffix) = suffix {
@@ -69,10 +46,7 @@ pub fn get_cache_path(
     Ok(path)
 }
 
-pub fn get_local_path(
-    local_path: PathBuf,
-    suffix: &Option<impl AsRef<Path>>,
-) -> Result<PathBuf, Error> {
+pub fn get_local_path(local_path: PathBuf, suffix: &Option<impl AsRef<Path>>) -> Result<PathBuf> {
     let mut path = local_path;
     create_dir(&path)?;
 
@@ -84,28 +58,28 @@ pub fn get_local_path(
     Ok(path)
 }
 
-pub fn get_indexedlogdatastore_path(path: impl AsRef<Path>) -> Result<PathBuf, Error> {
+pub fn get_indexedlogdatastore_path(path: impl AsRef<Path>) -> Result<PathBuf> {
     let mut path = path.as_ref().to_owned();
     path.push("indexedlogdatastore");
     create_shared_dir(&path)?;
     Ok(path)
 }
 
-pub fn get_indexedlogdatastore_aux_path(path: impl AsRef<Path>) -> Result<PathBuf, Error> {
+pub fn get_indexedlogdatastore_aux_path(path: impl AsRef<Path>) -> Result<PathBuf> {
     let mut path = path.as_ref().to_owned();
     path.push("indexedlogdatastore_aux");
     create_shared_dir(&path)?;
     Ok(path)
 }
 
-pub fn get_indexedloghistorystore_path(path: impl AsRef<Path>) -> Result<PathBuf, Error> {
+pub fn get_indexedloghistorystore_path(path: impl AsRef<Path>) -> Result<PathBuf> {
     let mut path = path.as_ref().to_owned();
     path.push("indexedloghistorystore");
     create_shared_dir(&path)?;
     Ok(path)
 }
 
-pub fn get_packs_path(path: impl AsRef<Path>, suffix: &Option<PathBuf>) -> Result<PathBuf, Error> {
+pub fn get_packs_path(path: impl AsRef<Path>, suffix: &Option<PathBuf>) -> Result<PathBuf> {
     let mut path = path.as_ref().to_owned();
     path.push("packs");
     create_shared_dir(&path)?;
@@ -118,14 +92,11 @@ pub fn get_packs_path(path: impl AsRef<Path>, suffix: &Option<PathBuf>) -> Resul
     Ok(path)
 }
 
-pub fn get_cache_packs_path(
-    config: &dyn Config,
-    suffix: &Option<PathBuf>,
-) -> Result<PathBuf, Error> {
+pub fn get_cache_packs_path(config: &dyn Config, suffix: &Option<PathBuf>) -> Result<PathBuf> {
     get_packs_path(get_config_cache_path(config)?, suffix)
 }
 
-fn get_lfs_path(store_path: impl AsRef<Path>) -> Result<PathBuf, Error> {
+fn get_lfs_path(store_path: impl AsRef<Path>) -> Result<PathBuf> {
     let mut path = store_path.as_ref().to_owned();
     path.push("lfs");
     create_shared_dir(&path)?;
@@ -133,7 +104,7 @@ fn get_lfs_path(store_path: impl AsRef<Path>) -> Result<PathBuf, Error> {
     Ok(path)
 }
 
-pub fn get_lfs_pointers_path(store_path: impl AsRef<Path>) -> Result<PathBuf, Error> {
+pub fn get_lfs_pointers_path(store_path: impl AsRef<Path>) -> Result<PathBuf> {
     let mut path = get_lfs_path(store_path)?;
     path.push("pointers");
     create_shared_dir(&path)?;
@@ -141,7 +112,7 @@ pub fn get_lfs_pointers_path(store_path: impl AsRef<Path>) -> Result<PathBuf, Er
     Ok(path)
 }
 
-pub fn get_lfs_objects_path(store_path: impl AsRef<Path>) -> Result<PathBuf, Error> {
+pub fn get_lfs_objects_path(store_path: impl AsRef<Path>) -> Result<PathBuf> {
     let mut path = get_lfs_path(store_path)?;
     path.push("objects");
     create_shared_dir(&path)?;
@@ -149,7 +120,7 @@ pub fn get_lfs_objects_path(store_path: impl AsRef<Path>) -> Result<PathBuf, Err
     Ok(path)
 }
 
-pub fn get_lfs_blobs_path(store_path: impl AsRef<Path>) -> Result<PathBuf, Error> {
+pub fn get_lfs_blobs_path(store_path: impl AsRef<Path>) -> Result<PathBuf> {
     let mut path = get_lfs_path(store_path)?;
     path.push("blobs");
     create_shared_dir(&path)?;

eden/scm/tests/test-fb-ext-remotefilelog-bad-configs.t

@@ -18,7 +18,7 @@ Verify error message when no cachepath specified
   $ cp $HGRCPATH $HGRCPATH.bak
   $ sed -i.bak -n "/cachepath/!p" $HGRCPATH
   $ hg up tip
-  abort: could not find config option remotefilelog.cachepath
+  abort: config remotefilelog.cachepath is not set
   [255]
   $ mv $HGRCPATH.bak $HGRCPATH