various refactors

Also

- add tests for ref-log file to assure it's not zero (but something more sensible)
- remove generic for convenience on most callsites

crates/gitbutler-core/src/ops/reflog.rs

@@ -174,6 +174,10 @@ mod set_target_ref {
 
         let contents = std::fs::read_to_string(&log_file_path)?;
         assert_eq!(reflog_lines(&contents).len(), 2);
+
+        let contents = std::fs::read_to_string(&log_file_path)?;
+        let lines = reflog_lines(&contents);
+        assert_signature(lines[0].signature);
         Ok(())
     }
 
@@ -222,12 +226,16 @@ mod set_target_ref {
         let oplog = git::Oid::from_str("0123456789abcdef0123456789abcdef0123456")?;
         set_reference_to_oplog(worktree_dir, commit_id.into(), oplog).expect("success");
 
-        let loose_ref_log_path = worktree_dir.join(".git/logs/refs/heads/gitbutler/target");
-        std::fs::remove_file(&loose_ref_log_path)?;
+        let log_file_path = worktree_dir.join(".git/logs/refs/heads/gitbutler/target");
+        std::fs::remove_file(&log_file_path)?;
 
         set_reference_to_oplog(worktree_dir, commit_id.into(), oplog)
             .expect("missing reflog files are recreated");
-        assert!(loose_ref_log_path.is_file(), "the file was recreated");
+        assert!(log_file_path.is_file(), "the file was recreated");
+
+        let contents = std::fs::read_to_string(&log_file_path)?;
+        let lines = reflog_lines(&contents);
+        assert_signature(lines[0].signature);
         Ok(())
     }
 
@@ -346,6 +354,10 @@ mod set_target_ref {
     fn assert_signature(sig: gix::actor::SignatureRef<'_>) {
         assert_eq!(sig.name, GITBUTLER_INTEGRATION_COMMIT_AUTHOR_NAME);
         assert_eq!(sig.email, GITBUTLER_INTEGRATION_COMMIT_AUTHOR_EMAIL);
+        assert_ne!(
+            sig.time.seconds, 0,
+            "we don't accidentally use the default time as it would caues GC as well"
+        );
     }
 
     fn setup_repo() -> anyhow::Result<(tempfile::TempDir, git2::Oid)> {

crates/gitbutler-core/src/project_repository/conflicts.rs

@@ -97,7 +97,7 @@ pub fn conflicting_files(repository: &Repository) -> Result<Vec<String>> {
 
 /// Check if `path` is conflicting in `repository`, or if `None`, check if there is any conflict.
 // TODO(ST): Should this not rather check the conflicting state in the index?
-pub fn is_conflicting<P: AsRef<Path>>(repository: &Repository, path: Option<P>) -> Result<bool> {
+pub fn is_conflicting(repository: &Repository, path: Option<&Path>) -> Result<bool> {
     let conflicts_path = repository.git_repository.path().join("conflicts");
     if !conflicts_path.exists() {
         return Ok(false);
@@ -105,11 +105,9 @@ pub fn is_conflicting<P: AsRef<Path>>(repository: &Repository, path: Option<P>)
 
     let file = std::fs::File::open(conflicts_path)?;
     let reader = std::io::BufReader::new(file);
+    // TODO(ST): This shouldn't work on UTF8 strings.
     let mut files = reader.lines().map_ok(PathBuf::from);
     if let Some(pathname) = path {
-        // TODO(ST): This shouldn't work on UTF8 strings.
-        let pathname = pathname.as_ref();
-
         // check if pathname is one of the lines in conflicts_path file
         for line in files {
             let line = line?;

crates/gitbutler-core/src/project_repository/signatures.rs

@@ -21,18 +21,11 @@ pub fn signatures<'a>(
     Ok((author, comitter))
 }
 
-fn try_from<'a>(value: &users::User) -> Result<git2::Signature<'a>, git::Error> {
-    if let Some(name) = &value.name {
-        git2::Signature::now(name, &value.email)
-            .map(Into::into)
-            .map_err(Into::into)
-    } else if let Some(name) = &value.given_name {
-        git2::Signature::now(name, &value.email)
-            .map(Into::into)
-            .map_err(Into::into)
-    } else {
-        git2::Signature::now(&value.email, &value.email)
-            .map(Into::into)
-            .map_err(Into::into)
-    }
+fn try_from(value: &users::User) -> Result<git2::Signature<'static>, git::Error> {
+    let name = value
+        .name
+        .as_deref()
+        .or(value.given_name.as_deref())
+        .unwrap_or(&value.email);
+    Ok(git2::Signature::now(name, &value.email)?)
 }

crates/gitbutler-core/src/virtual_branches/integration.rs

@@ -50,7 +50,7 @@ pub fn get_workspace_head(
     let target_commit = repo.find_commit(target.sha.into())?;
     let mut workspace_tree = target_commit.tree()?;
 
-    if conflicts::is_conflicting::<String>(project_repo, None)? {
+    if conflicts::is_conflicting(project_repo, None)? {
         let merge_parent =
             conflicts::merge_parent(project_repo)?.ok_or(anyhow!("No merge parent"))?;
         let first_branch = applied_branches.first().ok_or(anyhow!("No branches"))?;

crates/gitbutler-core/src/virtual_branches/virtual.rs

@@ -1167,7 +1167,7 @@ pub fn integrate_upstream_commits(
     branch_id: BranchId,
     user: Option<&users::User>,
 ) -> Result<(), anyhow::Error> {
-    conflicts::is_conflicting::<&Path>(project_repository, None)?;
+    conflicts::is_conflicting(project_repository, None)?;
 
     let repo = &project_repository.git_repository;
     let project = project_repository.project();
@@ -1979,7 +1979,7 @@ fn virtual_hunks_into_virtual_files(
         .map(|(path, hunks)| {
             let id = path.display().to_string();
             let conflicted =
-                conflicts::is_conflicting(project_repository, Some(&id)).unwrap_or(false);
+                conflicts::is_conflicting(project_repository, Some(id.as_ref())).unwrap_or(false);
             let binary = hunks.iter().any(|h| h.binary);
             let modified_at = hunks.iter().map(|h| h.modified_at).max().unwrap_or(0);
             debug_assert!(hunks.iter().all(|hunk| hunk.file_path == path));
@@ -2332,7 +2332,7 @@ pub fn commit(
 
     update_conflict_markers(project_repository, &files)?;
 
-    if conflicts::is_conflicting::<&Path>(project_repository, None)? {
+    if conflicts::is_conflicting(project_repository, None)? {
         return Err(errors::CommitError::Conflicted(errors::ProjectConflict {
             project_id: project_repository.project().id,
         }));
@@ -2943,7 +2943,7 @@ pub fn amend(
     commit_oid: git::Oid,
     target_ownership: &BranchOwnershipClaims,
 ) -> Result<git::Oid, errors::VirtualBranchError> {
-    if conflicts::is_conflicting::<&Path>(project_repository, None)? {
+    if conflicts::is_conflicting(project_repository, None)? {
         return Err(errors::VirtualBranchError::Conflict(
             errors::ProjectConflict {
                 project_id: project_repository.project().id,
@@ -3447,7 +3447,7 @@ pub fn cherry_pick(
     branch_id: BranchId,
     target_commit_oid: git::Oid,
 ) -> Result<Option<git::Oid>, errors::CherryPickError> {
-    if conflicts::is_conflicting::<&Path>(project_repository, None)? {
+    if conflicts::is_conflicting(project_repository, None)? {
         return Err(errors::CherryPickError::Conflict(errors::ProjectConflict {
             project_id: project_repository.project().id,
         }));
@@ -3630,7 +3630,7 @@ pub fn squash(
     branch_id: BranchId,
     commit_oid: git::Oid,
 ) -> Result<(), errors::SquashError> {
-    if conflicts::is_conflicting::<&Path>(project_repository, None)? {
+    if conflicts::is_conflicting(project_repository, None)? {
         return Err(errors::SquashError::Conflict(errors::ProjectConflict {
             project_id: project_repository.project().id,
         }));
@@ -3757,7 +3757,7 @@ pub fn update_commit_message(
         return Err(errors::UpdateCommitMessageError::EmptyMessage);
     }
 
-    if conflicts::is_conflicting::<&Path>(project_repository, None)? {
+    if conflicts::is_conflicting(project_repository, None)? {
         return Err(errors::UpdateCommitMessageError::Conflict(
             errors::ProjectConflict {
                 project_id: project_repository.project().id,