From 96382f279f290c8a570ed25c4f70bd2b9f08d1e1 Mon Sep 17 00:00:00 2001
From: Mattias Granlund <mtsgrd@gmail.com>
Date: Thu, 2 Jan 2025 19:46:48 +0100
Subject: [PATCH] Create re-usable file service for reading git blobs

- decouples reading from workspace from reading from commit
---
 apps/desktop/src/lib/file/FileDiff.svelte     | 30 +++----
 apps/desktop/src/lib/files/file.ts            |  6 ++
 apps/desktop/src/lib/files/fileService.ts     | 33 ++++++++
 apps/desktop/src/routes/+layout.ts            |  9 +-
 .../src/routes/[projectId]/+layout.svelte     |  2 +
 crates/gitbutler-repo/src/commands.rs         | 84 +++++++++----------
 crates/gitbutler-tauri/src/forge.rs           |  2 +-
 crates/gitbutler-tauri/src/main.rs            |  3 +-
 crates/gitbutler-tauri/src/repo.rs            | 21 +++--
 9 files changed, 115 insertions(+), 75 deletions(-)
 create mode 100644 apps/desktop/src/lib/files/file.ts
 create mode 100644 apps/desktop/src/lib/files/fileService.ts

diff --git a/apps/desktop/src/lib/file/FileDiff.svelte b/apps/desktop/src/lib/file/FileDiff.svelte
index d8a412bd80..6df1de8be7 100644
--- a/apps/desktop/src/lib/file/FileDiff.svelte
+++ b/apps/desktop/src/lib/file/FileDiff.svelte
@@ -1,6 +1,6 @@
 <script lang="ts">
-	import { invoke } from '$lib/backend/ipc';
 	import { Project } from '$lib/backend/projects';
+	import { FileService } from '$lib/files/fileService';
 	import HunkViewer from '$lib/hunk/HunkViewer.svelte';
 	import InfoMessage from '$lib/shared/InfoMessage.svelte';
 	import LargeDiffMessage from '$lib/shared/LargeDiffMessage.svelte';
@@ -8,15 +8,9 @@
 	import { getLocalCommits, getLocalAndRemoteCommits } from '$lib/vbranches/contexts';
 	import { getLockText } from '$lib/vbranches/tooltip';
 	import { getContext } from '@gitbutler/shared/context';
+	import type { FileInfo } from '$lib/files/file';
 	import type { HunkSection, ContentSection } from '$lib/utils/fileSections';
 
-	interface FileInfo {
-		content: string;
-		name?: string;
-		mimeType?: string;
-		size?: number;
-	}
-
 	interface Props {
 		filePath: string;
 		isBinary: boolean;
@@ -43,6 +37,7 @@
 
 	let alwaysShow = $state(false);
 	const project = getContext(Project);
+	const fileService = getContext(FileService);
 	const localCommits = isFileLocked ? getLocalCommits() : undefined;
 	const remoteCommits = isFileLocked ? getLocalAndRemoteCommits() : undefined;
 
@@ -80,18 +75,15 @@
 	});
 
 	async function fetchBlobInfo() {
+		if (!isBinary) {
+			return;
+		}
 		try {
-			const fetchedFileInfo: FileInfo = await invoke('get_blob_info', {
-				relativePath: filePath,
-				projectId: project.id,
-				commitId
-			});
-			fileInfo = fetchedFileInfo;
-
-			// If file.size > 5mb; don't render it
-			if (fileInfo.size && fileInfo.size > 5 * 1024 * 1024) {
-				isLarge = true;
-			}
+			const file = commitId
+				? await fileService.readFromCommit(filePath, project.id, commitId)
+				: await fileService.readFromWorkspace(filePath, project.id);
+			fileInfo = file.data;
+			isLarge = file.isLarge;
 		} catch (error) {
 			console.error(error);
 		}
diff --git a/apps/desktop/src/lib/files/file.ts b/apps/desktop/src/lib/files/file.ts
new file mode 100644
index 0000000000..ed8512e48b
--- /dev/null
+++ b/apps/desktop/src/lib/files/file.ts
@@ -0,0 +1,6 @@
+export type FileInfo = {
+	content: string;
+	name?: string;
+	mimeType?: string;
+	size?: number;
+};
diff --git a/apps/desktop/src/lib/files/fileService.ts b/apps/desktop/src/lib/files/fileService.ts
new file mode 100644
index 0000000000..cc3fdd00b6
--- /dev/null
+++ b/apps/desktop/src/lib/files/fileService.ts
@@ -0,0 +1,33 @@
+import type { Tauri } from '$lib/backend/tauri';
+import type { FileInfo } from './file';
+
+export class FileService {
+	constructor(private tauri: Tauri) {}
+
+	async readFromWorkspace(filePath: string, projectId: string) {
+		const data: FileInfo = await this.tauri.invoke('get_workspace_file', {
+			relativePath: filePath,
+			projectId: projectId
+		});
+		return {
+			data,
+			isLarge: isLarge(data.size)
+		};
+	}
+
+	async readFromCommit(filePath: string, projectId: string, commitId: string | undefined) {
+		const data: FileInfo = await this.tauri.invoke('get_commit_file', {
+			relativePath: filePath,
+			projectId: projectId,
+			commitId
+		});
+		return {
+			data,
+			isLarge: isLarge(data.size)
+		};
+	}
+}
+
+function isLarge(size: number | undefined) {
+	return size && size > 5 * 1024 * 1024 ? true : false;
+}
diff --git a/apps/desktop/src/routes/+layout.ts b/apps/desktop/src/routes/+layout.ts
index 50ac87faeb..8162b628a3 100644
--- a/apps/desktop/src/routes/+layout.ts
+++ b/apps/desktop/src/routes/+layout.ts
@@ -10,6 +10,7 @@ import { PromptService } from '$lib/backend/prompt';
 import { Tauri } from '$lib/backend/tauri';
 import { UpdaterService } from '$lib/backend/updater';
 import { loadAppSettings } from '$lib/config/appSettings';
+import { FileService } from '$lib/files/fileService';
 import { RemotesService } from '$lib/remotes/service';
 import { RustSecretService } from '$lib/secrets/secretsService';
 import { TokenMemoryService } from '$lib/stores/tokenMemoryService';
@@ -45,7 +46,8 @@ export const load: LayoutLoad = async () => {
 	const tokenMemoryService = new TokenMemoryService();
 	const httpClient = new HttpClient(window.fetch, PUBLIC_API_BASE_URL, tokenMemoryService.token);
 	const authService = new AuthService();
-	const updaterService = new UpdaterService(new Tauri(), posthog);
+	const tauri = new Tauri();
+	const updaterService = new UpdaterService(tauri, posthog);
 	const promptService = new PromptService();
 
 	const userService = new UserService(httpClient, tokenMemoryService, posthog);
@@ -59,6 +61,7 @@ export const load: LayoutLoad = async () => {
 	const aiPromptService = new AIPromptService();
 	const lineManagerFactory = new LineManagerFactory();
 	const stackingLineManagerFactory = new StackingLineManagerFactory();
+	const fileService = new FileService(tauri);
 
 	return {
 		commandService,
@@ -77,6 +80,8 @@ export const load: LayoutLoad = async () => {
 		lineManagerFactory,
 		stackingLineManagerFactory,
 		secretsService,
-		posthog
+		posthog,
+		tauri,
+		fileService
 	};
 };
diff --git a/apps/desktop/src/routes/[projectId]/+layout.svelte b/apps/desktop/src/routes/[projectId]/+layout.svelte
index c1d3477822..0cc3c3c77a 100644
--- a/apps/desktop/src/routes/[projectId]/+layout.svelte
+++ b/apps/desktop/src/routes/[projectId]/+layout.svelte
@@ -18,6 +18,7 @@
 	import { showHistoryView } from '$lib/config/config';
 	import { cloudFunctionality } from '$lib/config/uiFeatureFlags';
 	import { StackingReorderDropzoneManagerFactory } from '$lib/dragging/stackingReorderDropzoneManager';
+	import { FileService } from '$lib/files/fileService';
 	import { DefaultForgeFactory } from '$lib/forge/forgeFactory';
 	import { octokitFromAccessToken } from '$lib/forge/github/octokit';
 	import { createForgeStore } from '$lib/forge/interface/forge';
@@ -93,6 +94,7 @@
 		setContext(SyncedSnapshotService, data.syncedSnapshotService);
 		setContext(CloudBranchesService, data.cloudBranchesService);
 		setContext(CloudBranchCreationService, data.cloudBranchCreationService);
+		setContext(FileService, data.fileService);
 	});
 
 	const routesService = getRoutesService();
diff --git a/crates/gitbutler-repo/src/commands.rs b/crates/gitbutler-repo/src/commands.rs
index 1a99111725..f9a8005d84 100644
--- a/crates/gitbutler-repo/src/commands.rs
+++ b/crates/gitbutler-repo/src/commands.rs
@@ -102,7 +102,14 @@ pub trait RepoCommands {
     fn get_local_config(&self, key: &str) -> Result<Option<String>>;
     fn set_local_config(&self, key: &str, value: &str) -> Result<()>;
     fn check_signing_settings(&self) -> Result<bool>;
-    /// Read `probably_relative_path` in the following order:
+
+    /// Read `path` from the tree of the given commit.
+    ///
+    /// Bails when given an absolute path since that would suggest we are looking for a file in
+    /// the workspace. Returns `FileInfo::default()` if file could not be found.
+    fn read_file_from_commit(&self, commit_id: Oid, path: &Path) -> Result<FileInfo>;
+
+    /// Read `path` in the following order:
     ///
     /// * worktree
     /// * index
@@ -111,16 +118,8 @@ pub trait RepoCommands {
     /// This order makes sense if you imagine that deleted files are shown, like in a `git status`,
     /// so we want to know what's deleted.
     ///
-    /// If `probably_relative_path` is absolute, we will assure it's in the worktree.
-    /// If `treeish` is given, it will only be read from the given tree.
-    ///
-    /// If nothing could be found at `probably_relative_path`, the returned structure indicates this,
-    /// but it's no error.
-    fn read_file_from_workspace(
-        &self,
-        treeish: Option<Oid>,
-        probably_relative_path: &Path,
-    ) -> Result<FileInfo>;
+    /// Returns `FileInfo::default()` if file could not be found.
+    fn read_file_from_workspace(&self, path: &Path) -> Result<FileInfo>;
 }
 
 impl RepoCommands for Project {
@@ -182,23 +181,32 @@ impl RepoCommands for Project {
         Ok(())
     }
 
-    fn read_file_from_workspace(
-        &self,
-        treeish: Option<Oid>,
-        probably_relative_path: &Path,
-    ) -> Result<FileInfo> {
+    fn read_file_from_commit(&self, commit_id: Oid, relative_path: &Path) -> Result<FileInfo> {
+        if !relative_path.is_relative() {
+            bail!(
+                "Refusing to read '{:?}' from commit {:?} as it's not relative to the worktree",
+                relative_path,
+                commit_id
+            );
+        }
+
         let ctx = CommandContext::open(self)?;
         let repo = ctx.repo();
+        let tree = repo.find_commit(commit_id)?.tree()?;
 
-        if let Some(treeish) = treeish {
-            if !probably_relative_path.is_relative() {
-                bail!(
-                    "Refusing to read '{}' from tree as it's not relative to the worktree",
-                    probably_relative_path.display(),
-                );
+        Ok(match tree.get_path(relative_path) {
+            Ok(entry) => {
+                let blob = repo.find_blob(entry.id())?;
+                FileInfo::from_content(relative_path, blob.content())
             }
-            return read_file_from_tree(repo, Some(treeish), probably_relative_path);
-        }
+            Err(e) if e.code() == git2::ErrorCode::NotFound => FileInfo::deleted(),
+            Err(e) => return Err(e.into()),
+        })
+    }
+
+    fn read_file_from_workspace(&self, probably_relative_path: &Path) -> Result<FileInfo> {
+        let ctx = CommandContext::open(self)?;
+        let repo = ctx.repo();
 
         let (path_in_worktree, relative_path) = if probably_relative_path.is_relative() {
             (
@@ -234,34 +242,20 @@ impl RepoCommands for Project {
             }
             Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
                 match repo.index()?.get_path(&relative_path, 0) {
+                    // Read file that has been deleted and not staged for commit.
                     Some(entry) => {
                         let blob = repo.find_blob(entry.id)?;
                         FileInfo::from_content(&relative_path, blob.content())
                     }
-                    None => read_file_from_tree(repo, None, &relative_path)?,
+                    // Read file that has been deleted and staged for commit. Note that file not
+                    // found returns FileInfo::default() rather than an error.
+                    None => self.read_file_from_commit(
+                        repo.head()?.peel_to_commit()?.id(),
+                        &relative_path,
+                    )?,
                 }
             }
             Err(err) => return Err(err.into()),
         })
     }
 }
-
-fn read_file_from_tree(
-    repo: &git2::Repository,
-    treeish: Option<Oid>,
-    relative_path: &Path,
-) -> Result<FileInfo> {
-    let tree = if let Some(id) = treeish {
-        repo.find_object(id, None)?.peel_to_tree()?
-    } else {
-        repo.head()?.peel_to_tree()?
-    };
-    Ok(match tree.get_path(relative_path) {
-        Ok(entry) => {
-            let blob = repo.find_blob(entry.id())?;
-            FileInfo::from_content(relative_path, blob.content())
-        }
-        Err(e) if e.code() == git2::ErrorCode::NotFound => FileInfo::deleted(),
-        Err(e) => return Err(e.into()),
-    })
-}
diff --git a/crates/gitbutler-tauri/src/forge.rs b/crates/gitbutler-tauri/src/forge.rs
index ca0f4eeac1..1a06423b79 100644
--- a/crates/gitbutler-tauri/src/forge.rs
+++ b/crates/gitbutler-tauri/src/forge.rs
@@ -49,7 +49,7 @@ pub mod commands {
             .into());
         }
         Ok(project
-            .read_file_from_workspace(None, relative_path)?
+            .read_file_from_workspace(relative_path)?
             .content
             .context("PR template was not valid UTF-8")?)
     }
diff --git a/crates/gitbutler-tauri/src/main.rs b/crates/gitbutler-tauri/src/main.rs
index ac33c049d3..88795773a7 100644
--- a/crates/gitbutler-tauri/src/main.rs
+++ b/crates/gitbutler-tauri/src/main.rs
@@ -172,7 +172,8 @@ fn main() {
                     repo::commands::check_signing_settings,
                     repo::commands::git_clone_repository,
                     repo::commands::get_uncommited_files,
-                    repo::commands::get_blob_info,
+                    repo::commands::get_commit_file,
+                    repo::commands::get_workspace_file,
                     virtual_branches::commands::list_virtual_branches,
                     virtual_branches::commands::create_virtual_branch,
                     virtual_branches::commands::delete_local_branch,
diff --git a/crates/gitbutler-tauri/src/repo.rs b/crates/gitbutler-tauri/src/repo.rs
index 32e9f98b54..3163cfb474 100644
--- a/crates/gitbutler-tauri/src/repo.rs
+++ b/crates/gitbutler-tauri/src/repo.rs
@@ -1,7 +1,6 @@
 pub mod commands {
     use crate::error::{Error, UnmarkedError};
     use anyhow::Result;
-    use git2::Oid;
     use gitbutler_branch_actions::RemoteBranchFile;
     use gitbutler_project as projects;
     use gitbutler_project::ProjectId;
@@ -72,17 +71,25 @@ pub mod commands {
 
     #[tauri::command(async)]
     #[instrument(skip(projects))]
-    pub fn get_blob_info(
+    pub fn get_commit_file(
         projects: State<'_, projects::Controller>,
         project_id: ProjectId,
         relative_path: &Path,
-        commit_id: Option<String>,
+        commit_id: String,
     ) -> Result<FileInfo, Error> {
         let project = projects.get(project_id)?;
-        let commit_oid = commit_id
-            .map(|id| Oid::from_str(&id).map_err(|e| anyhow::anyhow!(e)))
-            .transpose()?;
+        let commit_oid = git2::Oid::from_str(commit_id.as_ref()).map_err(anyhow::Error::from)?;
+        Ok(project.read_file_from_commit(commit_oid, relative_path)?)
+    }
 
-        Ok(project.read_file_from_workspace(commit_oid, relative_path)?)
+    #[tauri::command(async)]
+    #[instrument(skip(projects))]
+    pub fn get_workspace_file(
+        projects: State<'_, projects::Controller>,
+        project_id: ProjectId,
+        relative_path: &Path,
+    ) -> Result<FileInfo, Error> {
+        let project = projects.get(project_id)?;
+        Ok(project.read_file_from_workspace(relative_path)?)
     }
 }