From 04ea5c9947e41e0ff403e6e5dda2be398d42da4e Mon Sep 17 00:00:00 2001 From: Mattias Granlund Date: Fri, 15 Nov 2024 18:57:21 +0000 Subject: [PATCH] Check for existing fork before adding new one - can prevent a cascading error - exposes remote urls to front end --- .../lib/components/PullRequestPreview.svelte | 2 +- apps/desktop/src/lib/remotes/service.ts | 26 ++++++++++--- crates/gitbutler-repo/src/commands.rs | 37 ++++++++++++++++--- crates/gitbutler-repo/src/lib.rs | 2 + crates/gitbutler-repo/src/remote.rs | 18 +++++++++ crates/gitbutler-tauri/src/remotes.rs | 11 +++--- 6 files changed, 78 insertions(+), 18 deletions(-) create mode 100644 crates/gitbutler-repo/src/remote.rs diff --git a/apps/desktop/src/lib/components/PullRequestPreview.svelte b/apps/desktop/src/lib/components/PullRequestPreview.svelte index e76274b40e..f5ad97908c 100644 --- a/apps/desktop/src/lib/components/PullRequestPreview.svelte +++ b/apps/desktop/src/lib/components/PullRequestPreview.svelte @@ -56,7 +56,7 @@ } const remotes = await remotesService.remotes(project.id); - if (remotes.includes(remoteName)) { + if (remotes.find((r) => r.name === remoteName)) { toasts.error('Remote already exists'); return; } diff --git a/apps/desktop/src/lib/remotes/service.ts b/apps/desktop/src/lib/remotes/service.ts index 7ed4f6e6cd..a31ba16df9 100644 --- a/apps/desktop/src/lib/remotes/service.ts +++ b/apps/desktop/src/lib/remotes/service.ts @@ -1,16 +1,30 @@ import { invoke } from '$lib/backend/ipc'; -import { showError } from '$lib/notifications/toasts'; + +export interface GitRemote { + name?: string; + url?: string; +} export class RemotesService { async remotes(projectId: string) { - return await invoke('list_remotes', { projectId }); + return await invoke('list_remotes', { projectId }); } async addRemote(projectId: string, name: string, url: string) { - try { - await invoke('add_remote', { projectId, name, url }); - } catch (e) { - showError('Failed to add remote', e); + const remotes = await this.remotes(projectId); + + const sameNameRemote = remotes.find((remote) => remote.name === name); + if (sameNameRemote) { + throw new Error(`Remote with name ${sameNameRemote.name} already exists.`); } + + const sameUrlRemote = remotes.find((remote) => remote.url === url); + if (sameUrlRemote) { + // This should not happen, and indicates we are incorrectly showing an "apply from fork" + // button in the user interface. + throw new Error(`Remote ${sameUrlRemote.name} with url ${sameUrlRemote.url} already exists.`); + } + + return await invoke('add_remote', { projectId, name, url }); } } diff --git a/crates/gitbutler-repo/src/commands.rs b/crates/gitbutler-repo/src/commands.rs index ac37f55a68..ba462ef824 100644 --- a/crates/gitbutler-repo/src/commands.rs +++ b/crates/gitbutler-repo/src/commands.rs @@ -1,10 +1,11 @@ -use crate::{Config, RepositoryExt}; +use crate::{remote::GitRemote, Config, RepositoryExt}; use anyhow::{bail, Result}; use base64::engine::Engine as _; use git2::Oid; use gitbutler_command_context::CommandContext; use gitbutler_project::Project; use infer::MatcherType; +use itertools::Itertools; use serde::Serialize; use std::path::Path; use tracing::warn; @@ -97,7 +98,7 @@ impl FileInfo { pub trait RepoCommands { fn add_remote(&self, name: &str, url: &str) -> Result<()>; - fn remotes(&self) -> Result>; + fn remotes(&self) -> Result>; fn get_local_config(&self, key: &str) -> Result>; fn set_local_config(&self, key: &str, value: &str) -> Result<()>; fn check_signing_settings(&self) -> Result; @@ -144,14 +145,40 @@ impl RepoCommands for Project { } } - fn remotes(&self) -> Result> { + fn remotes(&self) -> anyhow::Result> { let ctx = CommandContext::open(self)?; - ctx.repository().remotes_as_string() + let repo = ctx.repository(); + let remotes = repo + .remotes_as_string()? + .iter() + .map(|name| repo.find_remote(name)) + .collect::, _>>()? + .into_iter() + .map(|remote| remote.into()) + .collect_vec(); + Ok(remotes) } fn add_remote(&self, name: &str, url: &str) -> Result<()> { let ctx = CommandContext::open(self)?; - ctx.repository().remote(name, url)?; + let repo = ctx.repository(); + + // Bail if remote with given name already exists. + if repo.find_remote(name).is_ok() { + bail!("Remote name '{}' already exists", name); + } + + // Bail if remote with given url already exists. + if repo + .remotes_as_string()? + .iter() + .map(|name| repo.find_remote(name)) + .any(|result| result.is_ok_and(|remote| remote.url() == Some(url))) + { + bail!("Remote with url '{}' already exists", url); + } + + repo.remote(name, url)?; Ok(()) } diff --git a/crates/gitbutler-repo/src/lib.rs b/crates/gitbutler-repo/src/lib.rs index 937399e038..f0662deb91 100644 --- a/crates/gitbutler-repo/src/lib.rs +++ b/crates/gitbutler-repo/src/lib.rs @@ -2,6 +2,7 @@ pub mod rebase; mod commands; pub use commands::{FileInfo, RepoCommands}; +pub use remote::GitRemote; mod repository_ext; pub use repository_ext::{GixRepositoryExt, LogUntil, RepositoryExt}; @@ -9,6 +10,7 @@ pub use repository_ext::{GixRepositoryExt, LogUntil, RepositoryExt}; pub mod credentials; mod config; +mod remote; pub use config::Config; diff --git a/crates/gitbutler-repo/src/remote.rs b/crates/gitbutler-repo/src/remote.rs new file mode 100644 index 0000000000..beb935ff5a --- /dev/null +++ b/crates/gitbutler-repo/src/remote.rs @@ -0,0 +1,18 @@ +use serde::Serialize; + +/// Struct for exposing remote information to the front end. +#[derive(Default, Debug, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct GitRemote { + pub name: Option, + pub url: Option, +} + +impl From> for GitRemote { + fn from(value: git2::Remote) -> Self { + GitRemote { + name: value.name().map(|name| name.to_owned()), + url: value.url().map(|url| url.to_owned()), + } + } +} diff --git a/crates/gitbutler-tauri/src/remotes.rs b/crates/gitbutler-tauri/src/remotes.rs index e8f279978d..158d5f9ad0 100644 --- a/crates/gitbutler-tauri/src/remotes.rs +++ b/crates/gitbutler-tauri/src/remotes.rs @@ -1,19 +1,18 @@ +use crate::error::Error; use gitbutler_project as projects; use gitbutler_project::ProjectId; -use gitbutler_repo::RepoCommands; +use gitbutler_repo::{GitRemote, RepoCommands}; use tauri::State; use tracing::instrument; -use crate::error::Error; - #[tauri::command(async)] #[instrument(skip(projects), err(Debug))] pub fn list_remotes( projects: State<'_, projects::Controller>, project_id: ProjectId, -) -> Result, Error> { +) -> Result, Error> { let project = projects.get(project_id)?; - project.remotes().map_err(Into::into) + Ok(project.remotes()?) } #[tauri::command(async)] @@ -25,5 +24,5 @@ pub fn add_remote( url: &str, ) -> Result<(), Error> { let project = projects.get(project_id)?; - project.add_remote(name, url).map_err(Into::into) + Ok(project.add_remote(name, url)?) }