Use get_entry instead of get_str

CHANGELOG.md

@@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 - compilation broken on freebsd ([#461](https://github.com/extrawurst/gitui/issues/461))
+- don’t fail if `user.name` is not set [[@cruessler](https://github.com/cruessler)] ([#79](https://github.com/extrawurst/gitui/issues/79)) ([#228](https://github.com/extrawurst/gitui/issues/228))
 
 ## [0.11.0] - 2020-12-20
 

asyncgit/src/lib.rs

@@ -1,7 +1,7 @@
 //! asyncgit
 
-#![forbid(unsafe_code)]
 #![forbid(missing_docs)]
+#![deny(unsafe_code)]
 #![deny(unused_imports)]
 #![deny(clippy::all)]
 #![deny(clippy::unwrap_used)]

asyncgit/src/sync/commit.rs

@@ -36,17 +36,24 @@ pub fn amend(
 fn signature_allow_undefined_name(
     repo: &Repository,
 ) -> std::result::Result<Signature<'_>, git2::Error> {
-    match repo.signature() {
-        Err(e) if e.code() == ErrorCode::NotFound => {
+    let signature = repo.signature();
+
+    if let Err(ref e) = signature {
+        if e.code() == ErrorCode::NotFound {
             let config = repo.config()?;
-            Signature::now(
-                config.get_str("user.name").unwrap_or("unknown"),
-                config.get_str("user.email")?,
-            )
-        }
 
-        v => v,
+            if let (Err(_), Ok(email_entry)) = (
+                config.get_entry("user.name"),
+                config.get_entry("user.email"),
+            ) {
+                if let Some(email) = email_entry.value() {
+                    return Signature::now("unknown", email);
+                }
+            };
+        }
     }
+
+    signature
 }
 
 /// this does not run any git hooks
@@ -245,4 +252,86 @@ mod tests {
 
         Ok(())
     }
+
+    /// Beware: this test has to be run with a `$HOME/.gitconfig` that has
+    /// `user.email` not set. Otherwise, git falls back to the value of
+    /// `user.email` in `$HOME/.gitconfig` and this test fails.
+    ///
+    /// As of February 2021, `repo_init_empty` sets all git config locations
+    /// to an empty temporary directory, so this constraint is met.
+    #[test]
+    fn test_empty_email() -> Result<()> {
+        let file_path = Path::new("foo");
+        let (_td, repo) = repo_init_empty().unwrap();
+        let root = repo.path().parent().unwrap();
+        let repo_path = root.as_os_str().to_str().unwrap();
+
+        File::create(&root.join(file_path))?
+            .write_all(b"test\nfoo")?;
+
+        stage_add_file(repo_path, file_path)?;
+
+        repo.config()?.remove("user.email")?;
+
+        let error = commit(repo_path, "commit msg");
+
+        assert!(matches!(error, Err(_)));
+
+        repo.config()?.set_str("user.email", "email")?;
+
+        let success = commit(repo_path, "commit msg");
+
+        assert!(matches!(success, Ok(_)));
+        assert_eq!(count_commits(&repo, 10), 1);
+
+        let details =
+            get_commit_details(repo_path, success.unwrap()).unwrap();
+
+        assert_eq!(details.author.name, "name");
+        assert_eq!(details.author.email, "email");
+
+        Ok(())
+    }
+
+    /// See comment to `test_empty_email`.
+    #[test]
+    fn test_empty_name() -> Result<()> {
+        let file_path = Path::new("foo");
+        let (_td, repo) = repo_init_empty().unwrap();
+        let root = repo.path().parent().unwrap();
+        let repo_path = root.as_os_str().to_str().unwrap();
+
+        File::create(&root.join(file_path))?
+            .write_all(b"test\nfoo")?;
+
+        stage_add_file(repo_path, file_path)?;
+
+        repo.config()?.remove("user.name")?;
+
+        let mut success = commit(repo_path, "commit msg");
+
+        assert!(matches!(success, Ok(_)));
+        assert_eq!(count_commits(&repo, 10), 1);
+
+        let mut details =
+            get_commit_details(repo_path, success.unwrap()).unwrap();
+
+        assert_eq!(details.author.name, "unknown");
+        assert_eq!(details.author.email, "email");
+
+        repo.config()?.set_str("user.name", "name")?;
+
+        success = commit(repo_path, "commit msg");
+
+        assert!(matches!(success, Ok(_)));
+        assert_eq!(count_commits(&repo, 10), 2);
+
+        details =
+            get_commit_details(repo_path, success.unwrap()).unwrap();
+
+        assert_eq!(details.author.name, "name");
+        assert_eq!(details.author.email, "email");
+
+        Ok(())
+    }
 }

asyncgit/src/sync/mod.rs

@@ -60,8 +60,32 @@ mod tests {
     use std::process::Command;
     use tempfile::TempDir;
 
+    /// Calling `set_search_path` with an empty directory makes sure that there
+    /// is no git config interfering with our tests (for example user-local
+    /// `.gitconfig`).
+    #[allow(unsafe_code)]
+    fn sandbox_config_files() {
+        use git2::{opts::set_search_path, ConfigLevel};
+        use std::sync::Once;
+
+        static INIT: Once = Once::new();
+
+        // Adapted from https://github.com/rust-lang/cargo/pull/9035
+        INIT.call_once(|| unsafe {
+            let temp_dir = TempDir::new().unwrap();
+            let path = temp_dir.path();
+
+            set_search_path(ConfigLevel::System, &path).unwrap();
+            set_search_path(ConfigLevel::Global, &path).unwrap();
+            set_search_path(ConfigLevel::XDG, &path).unwrap();
+            set_search_path(ConfigLevel::ProgramData, &path).unwrap();
+        });
+    }
+
     ///
     pub fn repo_init_empty() -> Result<(TempDir, Repository)> {
+        sandbox_config_files();
+
         let td = TempDir::new()?;
         let repo = Repository::init(td.path())?;
         {
@@ -74,6 +98,8 @@ mod tests {
 
     ///
     pub fn repo_init() -> Result<(TempDir, Repository)> {
+        sandbox_config_files();
+
         let td = TempDir::new()?;
         let repo = Repository::init(td.path())?;
         {