Get default fetch remote from configuration (#2204)

fixes #1093

Author: cruessler

CHANGELOG.md

@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+### Fixes
+* respect configuration for remote when fetching (also applies to pulling) [[@cruessler](https://github.com/cruessler)] ([#1093](https://github.com/extrawurst/gitui/issues/1093))
+
 ## [0.26.0+1] - 2024-04-14
 
 **0.26.1** 

asyncgit/src/sync/cred.rs

@@ -2,6 +2,7 @@
 
 use super::{
 	remotes::{
+		get_default_remote_for_fetch_in_repo,
 		get_default_remote_for_push_in_repo,
 		get_default_remote_in_repo,
 	},
@@ -49,6 +50,26 @@ pub fn need_username_password(repo_path: &RepoPath) -> Result<bool> {
 }
 
 /// know if username and password are needed for this url
+/// TODO: Very similar to `need_username_password_for_fetch`. Can be refactored. See also
+/// `need_username_password`.
+pub fn need_username_password_for_fetch(
+	repo_path: &RepoPath,
+) -> Result<bool> {
+	let repo = repo(repo_path)?;
+	let remote = repo
+		.find_remote(&get_default_remote_for_fetch_in_repo(&repo)?)?;
+	let url = remote
+		.url()
+		.or_else(|| remote.url())
+		.ok_or(Error::UnknownRemote)?
+		.to_owned();
+	let is_http = url.starts_with("http");
+	Ok(is_http)
+}
+
+/// know if username and password are needed for this url
+/// TODO: Very similar to `need_username_password_for_fetch`. Can be refactored. See also
+/// `need_username_password`.
 pub fn need_username_password_for_push(
 	repo_path: &RepoPath,
 ) -> Result<bool> {
@@ -93,6 +114,36 @@ pub fn extract_username_password(
 }
 
 /// extract username and password
+/// TODO: Very similar to `extract_username_password_for_fetch`. Can be refactored.
+pub fn extract_username_password_for_fetch(
+	repo_path: &RepoPath,
+) -> Result<BasicAuthCredential> {
+	let repo = repo(repo_path)?;
+	let url = repo
+		.find_remote(&get_default_remote_for_fetch_in_repo(&repo)?)?
+		.url()
+		.ok_or(Error::UnknownRemote)?
+		.to_owned();
+	let mut helper = CredentialHelper::new(&url);
+
+	//TODO: look at Cred::credential_helper,
+	//if the username is in the url we need to set it here,
+	//I dont think `config` will pick it up
+
+	if let Ok(config) = repo.config() {
+		helper.config(&config);
+	}
+
+	Ok(match helper.execute() {
+		Some((username, password)) => {
+			BasicAuthCredential::new(Some(username), Some(password))
+		}
+		None => extract_cred_from_url(&url),
+	})
+}
+
+/// extract username and password
+/// TODO: Very similar to `extract_username_password_for_fetch`. Can be refactored.
 pub fn extract_username_password_for_push(
 	repo_path: &RepoPath,
 ) -> Result<BasicAuthCredential> {

asyncgit/src/sync/mod.rs

@@ -79,8 +79,9 @@ pub use merge::{
 };
 pub use rebase::rebase_branch;
 pub use remotes::{
-	get_default_remote, get_default_remote_for_push, get_remotes,
-	push::AsyncProgress, tags::PushTagsProgress,
+	get_default_remote, get_default_remote_for_fetch,
+	get_default_remote_for_push, get_remotes, push::AsyncProgress,
+	tags::PushTagsProgress,
 };
 pub(crate) use repository::repo;
 pub use repository::{RepoPath, RepoPathRef};

asyncgit/src/sync/remotes/mod.rs

@@ -66,6 +66,47 @@ fn get_current_branch(
 	Ok(None)
 }
 
+/// Tries to find the default repo to fetch from based on configuration.
+///
+/// > branch.<name>.remote
+/// >
+/// > When on branch `<name>`, it tells `git fetch` and `git push` which remote to fetch from or
+/// > push to. [...] If no remote is configured, or if you are not on any branch and there is more
+/// > than one remote defined in the repository, it defaults to `origin` for fetching [...].
+///
+/// [git-config-branch-name-remote]: https://git-scm.com/docs/git-config#Documentation/git-config.txt-branchltnamegtremote
+///
+/// Falls back to `get_default_remote_in_repo`.
+pub fn get_default_remote_for_fetch(
+	repo_path: &RepoPath,
+) -> Result<String> {
+	let repo = repo(repo_path)?;
+	get_default_remote_for_fetch_in_repo(&repo)
+}
+
+// TODO: Very similar to `get_default_remote_for_push_in_repo`. Can probably be refactored.
+pub(crate) fn get_default_remote_for_fetch_in_repo(
+	repo: &Repository,
+) -> Result<String> {
+	scope_time!("get_default_remote_for_fetch_in_repo");
+
+	let config = repo.config()?;
+
+	let branch = get_current_branch(repo)?;
+
+	if let Some(branch) = branch {
+		let remote_name = bytes2string(branch.name_bytes()?)?;
+
+		let entry_name = format!("branch.{}.remote", &remote_name);
+
+		if let Ok(entry) = config.get_entry(&entry_name) {
+			return bytes2string(entry.value_bytes());
+		}
+	}
+
+	get_default_remote_in_repo(repo)
+}
+
 /// Tries to find the default repo to push to based on configuration.
 ///
 /// > remote.pushDefault
@@ -93,6 +134,7 @@ pub fn get_default_remote_for_push(
 	get_default_remote_for_push_in_repo(&repo)
 }
 
+// TODO: Very similar to `get_default_remote_for_fetch_in_repo`. Can probably be refactored.
 pub(crate) fn get_default_remote_for_push_in_repo(
 	repo: &Repository,
 ) -> Result<String> {
@@ -383,6 +425,42 @@ mod tests {
 		));
 	}
 
+	#[test]
+	fn test_default_remote_for_fetch() {
+		let (remote_dir, _remote) = repo_init().unwrap();
+		let remote_path = remote_dir.path().to_str().unwrap();
+		let (repo_dir, repo) = repo_clone(remote_path).unwrap();
+		let repo_path: &RepoPath = &repo_dir
+			.into_path()
+			.as_os_str()
+			.to_str()
+			.unwrap()
+			.into();
+
+		debug_cmd_print(
+			repo_path,
+			"git remote rename origin alternate",
+		);
+
+		debug_cmd_print(
+			repo_path,
+			&format!("git remote add someremote {remote_path}")[..],
+		);
+
+		let mut config = repo.config().unwrap();
+
+		config
+			.set_str("branch.master.remote", "branchremote")
+			.unwrap();
+
+		let default_fetch_remote =
+			get_default_remote_for_fetch_in_repo(&repo);
+
+		assert!(
+			matches!(default_fetch_remote, Ok(remote_name) if remote_name == "branchremote")
+		);
+	}
+
 	#[test]
 	fn test_default_remote_for_push() {
 		let (remote_dir, _remote) = repo_init().unwrap();

src/popups/pull.rs

@@ -15,10 +15,11 @@ use asyncgit::{
 	sync::{
 		self,
 		cred::{
-			extract_username_password, need_username_password,
-			BasicAuthCredential,
+			extract_username_password_for_fetch,
+			need_username_password_for_fetch, BasicAuthCredential,
 		},
-		get_default_remote, RepoPathRef,
+		remotes::get_default_remote_for_fetch,
+		RepoPathRef,
 	},
 	AsyncGitNotification, AsyncPull, FetchRequest, RemoteProgress,
 };
@@ -69,11 +70,11 @@ impl PullPopup {
 	pub fn fetch(&mut self, branch: String) -> Result<()> {
 		self.branch = branch;
 		self.show()?;
-		if need_username_password(&self.repo.borrow())? {
-			let cred = extract_username_password(&self.repo.borrow())
-				.unwrap_or_else(|_| {
-					BasicAuthCredential::new(None, None)
-				});
+		if need_username_password_for_fetch(&self.repo.borrow())? {
+			let cred = extract_username_password_for_fetch(
+				&self.repo.borrow(),
+			)
+			.unwrap_or_else(|_| BasicAuthCredential::new(None, None));
 			if cred.is_complete() {
 				self.fetch_from_remote(Some(cred))
 			} else {
@@ -92,7 +93,9 @@ impl PullPopup {
 		self.pending = true;
 		self.progress = None;
 		self.git_fetch.request(FetchRequest {
-			remote: get_default_remote(&self.repo.borrow())?,
+			remote: get_default_remote_for_fetch(
+				&self.repo.borrow(),
+			)?,
 			branch: self.branch.clone(),
 			basic_credential: cred,
 		})?;

src/tabs/status.rs

@@ -58,7 +58,7 @@ enum DiffTarget {
 }
 
 struct RemoteStatus {
-	has_remotes: bool,
+	has_remote_for_fetch: bool,
 	has_remote_for_push: bool,
 }
 
@@ -161,7 +161,7 @@ impl Status {
 			queue: env.queue.clone(),
 			visible: true,
 			remotes: RemoteStatus {
-				has_remotes: false,
+				has_remote_for_fetch: false,
 				has_remote_for_push: false,
 			},
 			git_state: RepoState::Clean,
@@ -415,9 +415,11 @@ impl Status {
 	}
 
 	fn check_remotes(&mut self) {
-		self.remotes.has_remotes =
-			sync::get_default_remote(&self.repo.borrow().clone())
-				.is_ok();
+		self.remotes.has_remote_for_fetch =
+			sync::get_default_remote_for_fetch(
+				&self.repo.borrow().clone(),
+			)
+			.is_ok();
 		self.remotes.has_remote_for_push =
 			sync::get_default_remote_for_push(
 				&self.repo.borrow().clone(),
@@ -580,7 +582,7 @@ impl Status {
 	}
 
 	fn fetch(&self) {
-		if self.can_pull() {
+		if self.can_fetch() {
 			self.queue.push(InternalEvent::FetchRemotes);
 		}
 	}
@@ -616,8 +618,9 @@ impl Status {
 		is_ahead && self.remotes.has_remote_for_push
 	}
 
-	const fn can_pull(&self) -> bool {
-		self.remotes.has_remotes && self.git_branch_state.is_some()
+	const fn can_fetch(&self) -> bool {
+		self.remotes.has_remote_for_fetch
+			&& self.git_branch_state.is_some()
 	}
 
 	fn can_abort_merge(&self) -> bool {
@@ -754,12 +757,12 @@ impl Component for Status {
 
 			out.push(CommandInfo::new(
 				strings::commands::status_fetch(&self.key_config),
-				self.can_pull(),
+				self.can_fetch(),
 				!focus_on_diff,
 			));
 			out.push(CommandInfo::new(
 				strings::commands::status_pull(&self.key_config),
-				self.can_pull(),
+				self.can_fetch(),
 				!focus_on_diff,
 			));
 
@@ -881,13 +884,13 @@ impl Component for Status {
 					Ok(EventState::Consumed)
 				} else if key_match(k, self.key_config.keys.fetch)
 					&& !self.is_focus_on_diff()
-					&& self.can_pull()
+					&& self.can_fetch()
 				{
 					self.fetch();
 					Ok(EventState::Consumed)
 				} else if key_match(k, self.key_config.keys.pull)
 					&& !self.is_focus_on_diff()
-					&& self.can_pull()
+					&& self.can_fetch()
 				{
 					self.pull();
 					Ok(EventState::Consumed)