From cef463ff2d7245e3c5afddf20912bfda50088b7d Mon Sep 17 00:00:00 2001 From: Stephan Dilly Date: Wed, 24 Feb 2021 10:26:25 +0100 Subject: [PATCH 1/2] prefer 'origin' as default remote if it exists (closes #494) --- CHANGELOG.md | 3 ++- asyncgit/src/error.rs | 3 +++ asyncgit/src/sync/branch.rs | 6 +++-- asyncgit/src/sync/cred.rs | 14 +++++------ asyncgit/src/sync/mod.rs | 2 +- asyncgit/src/sync/remotes.rs | 45 +++++++++++++++++++++++++----------- src/components/push.rs | 4 ++-- 7 files changed, 50 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9214754f00..1ebbac317d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,9 +12,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - support mouse scrolling [[@WizardOhio24](https://github.com/WizardOhio24)] ([#306](https://github.com/extrawurst/gitui/issues/306)) - show used char count in input texts ([#466](https://github.com/extrawurst/gitui/issues/466)) -![push](assets/char_count.gif) +![charcount](assets/char_count.gif) ### Fixed +- push defaults to 'origin' remote if it exists ([#494](https://github.com/extrawurst/gitui/issues/494)) - support missing pageUp/down support in branchlist ([#519](https://github.com/extrawurst/gitui/issues/519)) - don't hide branch name while in commit dialog ([#529](https://github.com/extrawurst/gitui/issues/529)) - don't discard commit message without confirmation ([#530](https://github.com/extrawurst/gitui/issues/530)) diff --git a/asyncgit/src/error.rs b/asyncgit/src/error.rs index 989a2416f1..d502a5fd51 100644 --- a/asyncgit/src/error.rs +++ b/asyncgit/src/error.rs @@ -12,6 +12,9 @@ pub enum Error { #[error("git: remote url not found")] UnknownRemote, + #[error("git: inconclusive remotes")] + NoDefaultRemoteFound, + #[error("git: work dir error")] NoWorkDir, diff --git a/asyncgit/src/sync/branch.rs b/asyncgit/src/sync/branch.rs index 67455254b2..81fc7c97d2 100644 --- a/asyncgit/src/sync/branch.rs +++ b/asyncgit/src/sync/branch.rs @@ -1,6 +1,8 @@ //! -use super::{remotes::get_first_remote_in_repo, utils::bytes2string}; +use super::{ + remotes::get_default_remote_in_repo, utils::bytes2string, +}; use crate::{ error::{Error, Result}, sync::{utils, CommitId}, @@ -97,7 +99,7 @@ pub(crate) fn branch_set_upstream( repo.find_branch(branch_name, BranchType::Local)?; if branch.upstream().is_err() { - let remote = get_first_remote_in_repo(repo)?; + let remote = get_default_remote_in_repo(repo)?; let upstream_name = format!("{}/{}", remote, branch_name); branch.set_upstream(Some(upstream_name.as_str()))?; } diff --git a/asyncgit/src/sync/cred.rs b/asyncgit/src/sync/cred.rs index 8d8706a74d..cb7b7f34e3 100644 --- a/asyncgit/src/sync/cred.rs +++ b/asyncgit/src/sync/cred.rs @@ -1,12 +1,12 @@ //! credentials git helper +use super::remotes::get_default_remote_in_repo; +use crate::{ + error::{Error, Result}, + CWD, +}; use git2::{Config, CredentialHelper}; -use crate::error::{Error, Result}; -use crate::CWD; - -use super::remotes::get_first_remote_in_repo; - /// basic Authentication Credentials #[derive(Debug, Clone, Default, PartialEq)] pub struct BasicAuthCredential { @@ -34,7 +34,7 @@ impl BasicAuthCredential { pub fn need_username_password() -> Result { let repo = crate::sync::utils::repo(CWD)?; let url = repo - .find_remote(&get_first_remote_in_repo(&repo)?)? + .find_remote(&get_default_remote_in_repo(&repo)?)? .url() .ok_or(Error::UnknownRemote)? .to_owned(); @@ -46,7 +46,7 @@ pub fn need_username_password() -> Result { pub fn extract_username_password() -> Result { let repo = crate::sync::utils::repo(CWD)?; let url = repo - .find_remote(&get_first_remote_in_repo(&repo)?)? + .find_remote(&get_default_remote_in_repo(&repo)?)? .url() .ok_or(Error::UnknownRemote)? .to_owned(); diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index 96c8cbd070..57c9235d02 100644 --- a/asyncgit/src/sync/mod.rs +++ b/asyncgit/src/sync/mod.rs @@ -41,7 +41,7 @@ pub use hunks::{reset_hunk, stage_hunk, unstage_hunk}; pub use ignore::add_to_ignore; pub use logwalker::LogWalker; pub use remotes::{ - fetch_origin, get_first_remote, get_remotes, push, + fetch_origin, get_default_remote, get_remotes, push, ProgressNotification, }; pub use reset::{reset_stage, reset_workdir}; diff --git a/asyncgit/src/sync/remotes.rs b/asyncgit/src/sync/remotes.rs index 59582ee760..900b717b3a 100644 --- a/asyncgit/src/sync/remotes.rs +++ b/asyncgit/src/sync/remotes.rs @@ -66,28 +66,45 @@ pub fn get_remotes(repo_path: &str) -> Result> { Ok(remotes) } -/// -pub fn get_first_remote(repo_path: &str) -> Result { +/// tries to find origin or the only remote that is defined if any +/// in case of multiple remotes and none named *origin* we fail +pub fn get_default_remote(repo_path: &str) -> Result { let repo = utils::repo(repo_path)?; - get_first_remote_in_repo(&repo) + get_default_remote_in_repo(&repo) } -/// -pub(crate) fn get_first_remote_in_repo( +/// see `get_default_remote` +pub(crate) fn get_default_remote_in_repo( repo: &Repository, ) -> Result { - scope_time!("get_remotes"); + scope_time!("get_default_remote_in_repo"); let remotes = repo.remotes()?; - let first_remote = remotes + // if `origin` exists return that + if remotes .iter() - .next() - .flatten() - .map(String::from) - .ok_or_else(|| Error::Generic("no remote found".into()))?; + .any(|r| r.map(|r| r == "origin").unwrap_or_default()) + { + return Ok("origin".into()); + } + + //if only one remote exists pick that + if remotes.len() == 1 { + let first_remote = remotes + .iter() + .next() + .flatten() + .map(String::from) + .ok_or_else(|| { + Error::Generic("no remote found".into()) + })?; + + return Ok(first_remote); + } - Ok(first_remote) + //inconclusive + Err(Error::NoDefaultRemoteFound) } /// @@ -96,7 +113,7 @@ pub fn fetch_origin(repo_path: &str, branch: &str) -> Result { let repo = utils::repo(repo_path)?; let mut remote = - repo.find_remote(&get_first_remote_in_repo(&repo)?)?; + repo.find_remote(&get_default_remote_in_repo(&repo)?)?; let mut options = FetchOptions::new(); options.remote_callbacks(remote_callbacks(None, None)); @@ -311,7 +328,7 @@ mod tests { vec![String::from("origin"), String::from("second")] ); - let first = get_first_remote_in_repo( + let first = get_default_remote_in_repo( &utils::repo(repo_path).unwrap(), ) .unwrap(); diff --git a/src/components/push.rs b/src/components/push.rs index f217d7c9ee..68fa6c8cb6 100644 --- a/src/components/push.rs +++ b/src/components/push.rs @@ -15,7 +15,7 @@ use asyncgit::{ extract_username_password, need_username_password, BasicAuthCredential, }, - get_first_remote, + get_default_remote, }, AsyncNotification, AsyncPush, PushProgress, PushProgressState, PushRequest, CWD, @@ -102,7 +102,7 @@ impl PushComponent { self.pending = true; self.progress = None; self.git_push.request(PushRequest { - remote: get_first_remote(CWD)?, + remote: get_default_remote(CWD)?, branch: self.branch.clone(), force, basic_credential: cred, From 609a894b8b24261061a9a87dc30d7a67ce469e10 Mon Sep 17 00:00:00 2001 From: Stephan Dilly Date: Wed, 24 Feb 2021 10:44:55 +0100 Subject: [PATCH 2/2] unittest --- asyncgit/src/sync/cred.rs | 13 ++++----- asyncgit/src/sync/remotes.rs | 51 +++++++++++++++++++++++++++++++----- 2 files changed, 52 insertions(+), 12 deletions(-) diff --git a/asyncgit/src/sync/cred.rs b/asyncgit/src/sync/cred.rs index cb7b7f34e3..c769082335 100644 --- a/asyncgit/src/sync/cred.rs +++ b/asyncgit/src/sync/cred.rs @@ -81,16 +81,17 @@ pub fn extract_cred_from_url(url: &str) -> BasicAuthCredential { #[cfg(test)] mod tests { - use crate::sync::cred::{ - extract_cred_from_url, extract_username_password, - need_username_password, BasicAuthCredential, + use crate::sync::{ + cred::{ + extract_cred_from_url, extract_username_password, + need_username_password, BasicAuthCredential, + }, + remotes::DEFAULT_REMOTE_NAME, + tests::repo_init, }; - use crate::sync::tests::repo_init; use serial_test::serial; use std::env; - const DEFAULT_REMOTE_NAME: &str = "origin"; - #[test] fn test_credential_complete() { assert_eq!( diff --git a/asyncgit/src/sync/remotes.rs b/asyncgit/src/sync/remotes.rs index 900b717b3a..05c446c999 100644 --- a/asyncgit/src/sync/remotes.rs +++ b/asyncgit/src/sync/remotes.rs @@ -13,6 +13,8 @@ use git2::{ }; use scopetime::scope_time; +pub const DEFAULT_REMOTE_NAME: &str = "origin"; + /// #[derive(Debug, Clone)] pub enum ProgressNotification { @@ -82,11 +84,11 @@ pub(crate) fn get_default_remote_in_repo( let remotes = repo.remotes()?; // if `origin` exists return that - if remotes - .iter() - .any(|r| r.map(|r| r == "origin").unwrap_or_default()) - { - return Ok("origin".into()); + let found_origin = remotes.iter().any(|r| { + r.map(|r| r == DEFAULT_REMOTE_NAME).unwrap_or_default() + }); + if found_origin { + return Ok(DEFAULT_REMOTE_NAME.into()); } //if only one remote exists pick that @@ -305,7 +307,7 @@ mod tests { } #[test] - fn test_first_remote() { + fn test_default_remote() { let td = TempDir::new().unwrap(); debug_cmd_print( @@ -335,6 +337,43 @@ mod tests { assert_eq!(first, String::from("origin")); } + #[test] + fn test_default_remote_out_of_order() { + let td = TempDir::new().unwrap(); + + debug_cmd_print( + td.path().as_os_str().to_str().unwrap(), + "git clone https://github.com/extrawurst/brewdump.git", + ); + + debug_cmd_print( + td.path().as_os_str().to_str().unwrap(), + "cd brewdump && git remote rename origin alternate", + ); + + debug_cmd_print( + td.path().as_os_str().to_str().unwrap(), + "cd brewdump && git remote add origin https://github.com/extrawurst/brewdump.git", + ); + + let repo_path = td.path().join("brewdump"); + let repo_path = repo_path.as_os_str().to_str().unwrap(); + + //NOTE: aparently remotes are not chronolically sorted but alphabetically + let remotes = get_remotes(repo_path).unwrap(); + + assert_eq!( + remotes, + vec![String::from("alternate"), String::from("origin")] + ); + + let first = get_default_remote_in_repo( + &utils::repo(repo_path).unwrap(), + ) + .unwrap(); + assert_eq!(first, String::from("origin")); + } + #[test] fn test_force_push() { // This test mimics the scenario of 2 people having 2