fix!: rename Repository::branch_remote_ref() to Repository::branch_remote_ref_name(), add direction argument (also to Repository::branch_remote_name() and Repository::branch_remote()).

This better differentiates the return value from the corresponding ref objects,
which would require the named ref to exist in the repository.

The `direction` argument allows to get the reference to push to as well.
Further, it now takes a full ref name to support deriving the name of branches
to push to.

Regarding `Repository::branch_remote()`,  previously, this functionality
was only available from a `Reference`,
but now it's more generally available with just a branch name.

The method was also adjusted to permit looking up non-symbolic remote
names, like remotes that are specified by their URL.

Author: Byron

gix/src/reference/remote.rs

@@ -1,8 +1,8 @@
-use crate::{config, config::tree::Branch, remote, Reference};
+use crate::{remote, Reference};
 
 /// Remotes
 impl<'repo> Reference<'repo> {
-    /// Find the unvalidated name of our remote for `direction` as configured in `branch.<name>.remote|pushRemote` respectively.
+    /// Find the name of our remote for `direction` as configured in `branch.<name>.remote|pushRemote` respectively.
     /// If `Some(<name>)` it can be used in [`Repository::find_remote(…)`][crate::Repository::find_remote()], or if `None` then
     /// [`Repository::remote_default_name()`][crate::Repository::remote_default_name()] could be used in its place.
     ///
@@ -14,36 +14,15 @@ impl<'repo> Reference<'repo> {
     ///   information.
     /// - `branch.<name>.pushRemote` falls back to `branch.<name>.remote`.
     pub fn remote_name(&self, direction: remote::Direction) -> Option<remote::Name<'repo>> {
-        let name = self.name().shorten();
-        let config = &self.repo.config.resolved;
-        (direction == remote::Direction::Push)
-            .then(|| {
-                config
-                    .string("branch", Some(name), Branch::PUSH_REMOTE.name)
-                    .or_else(|| config.string("remote", None, config::tree::Remote::PUSH_DEFAULT.name))
-            })
-            .flatten()
-            .or_else(|| config.string("branch", Some(name), Branch::REMOTE.name))
-            .and_then(|name| name.try_into().ok())
+        self.repo.branch_remote_name(self.name().shorten(), direction)
     }
 
-    /// Like [`remote_name(…)`][Self::remote_name()], but configures the returned `Remote` with additional information like
-    ///
-    /// - `branch.<name>.merge` to know which branch on the remote side corresponds to this one for merging when pulling.
-    ///
-    /// It also handles if the remote is a configured URL, which has no name.
+    /// Like [`branch_remote(…)`](crate::Repository::branch_remote()), but automatically provides the reference name
+    /// for configuration lookup.
     pub fn remote(
         &self,
         direction: remote::Direction,
     ) -> Option<Result<crate::Remote<'repo>, remote::find::existing::Error>> {
-        // TODO: use `branch.<name>.merge`
-        self.remote_name(direction).map(|name| match name {
-            remote::Name::Symbol(name) => self.repo.find_remote(name.as_ref()).map_err(Into::into),
-            remote::Name::Url(url) => gix_url::parse(url.as_ref()).map_err(Into::into).and_then(|url| {
-                self.repo
-                    .remote_at(url)
-                    .map_err(|err| remote::find::existing::Error::Find(remote::find::Error::Init(err)))
-            }),
-        })
+        self.repo.branch_remote(self.name().shorten(), direction)
     }
 }

gix/src/repository/config/mod.rs

@@ -138,15 +138,17 @@ mod remote {
     use crate::bstr::BStr;
     use std::{borrow::Cow, collections::BTreeSet};
 
+    use crate::config::tree::{Remote, Section};
     use crate::remote;
 
+    /// Query configuration related to remotes.
     impl crate::Repository {
         /// Returns a sorted list unique of symbolic names of remotes that
         /// we deem [trustworthy][crate::open::Options::filter_config_section()].
         pub fn remote_names(&self) -> BTreeSet<Cow<'_, BStr>> {
             self.config
                 .resolved
-                .sections_by_name("remote")
+                .sections_by_name(Remote.name())
                 .map(|it| {
                     let filter = self.filter_config_section();
                     it.filter(move |s| filter(s.meta()))
@@ -167,9 +169,12 @@ mod remote {
         pub fn remote_default_name(&self, direction: remote::Direction) -> Option<Cow<'_, BStr>> {
             let name = (direction == remote::Direction::Push)
                 .then(|| {
-                    self.config
-                        .resolved
-                        .string_filter("remote", None, "pushDefault", &mut self.filter_config_section())
+                    self.config.resolved.string_filter(
+                        Remote.name(),
+                        None,
+                        Remote::PUSH_DEFAULT.name,
+                        &mut self.filter_config_section(),
+                    )
                 })
                 .flatten();
             name.or_else(|| {
@@ -190,11 +195,15 @@ mod remote {
 mod branch {
     use std::{borrow::Cow, collections::BTreeSet, convert::TryInto};
 
-    use gix_ref::FullNameRef;
-    use gix_validate::reference::name::Error as ValidateNameError;
+    use gix_ref::{FullName, FullNameRef};
 
     use crate::bstr::BStr;
+    use crate::config::cache::util::ApplyLeniencyDefault;
+    use crate::config::tree::{Branch, Push, Section};
+    use crate::repository::branch_remote_ref_name;
+    use crate::{push, remote};
 
+    /// Query configuration related to branches.
     impl crate::Repository {
         /// Return a set of unique short branch names for which custom configuration exists in the configuration,
         /// if we deem them [trustworthy][crate::open::Options::filter_config_section()].
@@ -206,38 +215,143 @@ mod branch {
             self.subsection_str_names_of("branch")
         }
 
-        /// Returns the validated reference on the remote associated with the given `short_branch_name`,
-        /// always `main` instead of `refs/heads/main`.
+        /// Returns the validated reference on the remote associated with the given `name`,
+        /// which will be used when *merging*.
+        /// The returned value corresponds to the `branch.<short_branch_name>.merge` configuration key.
+        ///
+        /// Returns `None` if there is no value at the given key, or if no remote or remote ref is configured.
+        /// May return an error if the reference name to be returned is invalid.
         ///
-        /// The returned reference is the one we track on the remote side for merging and pushing.
-        /// Returns `None` if the remote reference was not found.
-        /// May return an error if the reference is invalid.
-        pub fn branch_remote_ref<'a>(
+        /// ### Note
+        ///
+        /// This name refers to what Git calls upstream branch (as opposed to upstream *tracking* branch).
+        #[doc(alias = "branch_upstream_name", alias = "git2")]
+        pub fn branch_remote_ref_name(
             &self,
-            short_branch_name: impl Into<&'a BStr>,
-        ) -> Option<Result<Cow<'_, FullNameRef>, ValidateNameError>> {
-            self.config
-                .resolved
-                .string("branch", Some(short_branch_name.into()), "merge")
-                .map(crate::config::tree::branch::Merge::try_into_fullrefname)
+            name: &FullNameRef,
+            direction: remote::Direction,
+        ) -> Option<Result<Cow<'_, FullNameRef>, branch_remote_ref_name::Error>> {
+            match direction {
+                remote::Direction::Fetch => {
+                    let short_name = name.shorten();
+                    self.config
+                        .resolved
+                        .string("branch", Some(short_name), Branch::MERGE.name)
+                        .map(|name| crate::config::tree::branch::Merge::try_into_fullrefname(name).map_err(Into::into))
+                }
+                remote::Direction::Push => {
+                    let remote = match self.branch_remote(name.shorten(), direction)? {
+                        Ok(r) => r,
+                        Err(err) => return Some(Err(err.into())),
+                    };
+                    if remote.push_specs.is_empty() {
+                        let push_default = match self
+                            .config
+                            .resolved
+                            .string(Push.name(), None, Push::DEFAULT.name)
+                            .map_or(Ok(Default::default()), |v| {
+                                Push::DEFAULT
+                                    .try_into_default(v)
+                                    .with_lenient_default(self.config.lenient_config)
+                            }) {
+                            Ok(v) => v,
+                            Err(err) => return Some(Err(err.into())),
+                        };
+                        match push_default {
+                            push::Default::Nothing => None,
+                            push::Default::Current | push::Default::Matching => Some(Ok(Cow::Owned(name.to_owned()))),
+                            push::Default::Upstream => self.branch_remote_ref_name(name, remote::Direction::Fetch),
+                            push::Default::Simple => {
+                                match self.branch_remote_ref_name(name, remote::Direction::Fetch)? {
+                                    Ok(fetch_ref) if fetch_ref.as_ref() == name => Some(Ok(fetch_ref)),
+                                    Err(err) => Some(Err(err)),
+                                    Ok(_different_fetch_ref) => None,
+                                }
+                            }
+                        }
+                    } else {
+                        let search = gix_refspec::MatchGroup::from_push_specs(
+                            remote
+                                .push_specs
+                                .iter()
+                                .map(gix_refspec::RefSpec::to_ref)
+                                .filter(|spec| spec.destination().is_some()),
+                        );
+                        let null_id = self.object_hash().null();
+                        let out = search.match_remotes(
+                            Some(gix_refspec::match_group::Item {
+                                full_ref_name: name.as_bstr(),
+                                target: &null_id,
+                                object: None,
+                            })
+                            .into_iter(),
+                        );
+                        out.mappings.into_iter().next().and_then(|m| {
+                            m.rhs.map(|name| {
+                                FullName::try_from(name.into_owned())
+                                    .map(Cow::Owned)
+                                    .map_err(Into::into)
+                            })
+                        })
+                    }
+                }
+            }
         }
 
         /// Returns the unvalidated name of the remote associated with the given `short_branch_name`,
         /// typically `main` instead of `refs/heads/main`.
         /// In some cases, the returned name will be an URL.
         /// Returns `None` if the remote was not found or if the name contained illformed UTF-8.
         ///
+        /// * if `direction` is [remote::Direction::Fetch], we will query the `branch.<short_name>.remote` configuration.
+        /// * if `direction` is [remote::Direction::Push], the push remote will be queried by means of `branch.<short_name>.pushRemote`
+        ///   or `remote.pushDefault` as fallback.
+        ///
         /// See also [`Reference::remote_name()`][crate::Reference::remote_name()] for a more typesafe version
         /// to be used when a `Reference` is available.
+        ///
+        /// `short_branch_name` can typically be obtained by [shortening a full branch name](FullNameRef::shorten()).
+        #[doc(alias = "branch_upstream_remote", alias = "git2")]
         pub fn branch_remote_name<'a>(
             &self,
             short_branch_name: impl Into<&'a BStr>,
-        ) -> Option<crate::remote::Name<'_>> {
-            self.config
-                .resolved
-                .string("branch", Some(short_branch_name.into()), "remote")
+            direction: remote::Direction,
+        ) -> Option<remote::Name<'_>> {
+            let name = short_branch_name.into();
+            let config = &self.config.resolved;
+            (direction == remote::Direction::Push)
+                .then(|| {
+                    config
+                        .string("branch", Some(name), Branch::PUSH_REMOTE.name)
+                        .or_else(|| config.string("remote", None, crate::config::tree::Remote::PUSH_DEFAULT.name))
+                })
+                .flatten()
+                .or_else(|| config.string("branch", Some(name), Branch::REMOTE.name))
                 .and_then(|name| name.try_into().ok())
         }
+
+        /// Like [`branch_remote_name(…)`](Self::branch_remote_name()), but returns a [Remote](crate::Remote).
+        /// `short_branch_name` is the name to use for looking up `branch.<short_branch_name>.*` values in the
+        /// configuration.
+        pub fn branch_remote<'a>(
+            &self,
+            short_branch_name: impl Into<&'a BStr>,
+            direction: remote::Direction,
+        ) -> Option<Result<crate::Remote<'_>, remote::find::existing::Error>> {
+            let name = self.branch_remote_name(short_branch_name, direction)?;
+            self.try_find_remote(name.as_bstr())
+                .map(|res| res.map_err(Into::into))
+                .or_else(|| match name {
+                    remote::Name::Url(url) => gix_url::parse(url.as_ref())
+                        .map_err(Into::into)
+                        .and_then(|url| {
+                            self.remote_at(url)
+                                .map_err(|err| remote::find::existing::Error::Find(remote::find::Error::Init(err)))
+                        })
+                        .into(),
+                    remote::Name::Symbol(_) => None,
+                })
+        }
     }
 }
 

gix/src/repository/mod.rs

@@ -69,6 +69,22 @@ mod submodule;
 mod thread_safe;
 mod worktree;
 
+///
+pub mod branch_remote_ref_name {
+
+    /// The error returned by [Repository::branch_remote_ref_name()](crate::Repository::branch_remote_ref_name()).
+    #[derive(Debug, thiserror::Error)]
+    #[allow(missing_docs)]
+    pub enum Error {
+        #[error("The configured name of the remote ref to merge wasn't valid")]
+        ValidateFetchRemoteRefName(#[from] gix_validate::reference::name::Error),
+        #[error(transparent)]
+        PushDefault(#[from] crate::config::key::GenericErrorWithValue),
+        #[error(transparent)]
+        FindPushRemote(#[from] crate::remote::find::existing::Error),
+    }
+}
+
 /// A type to represent an index which either was loaded from disk as it was persisted there, or created on the fly in memory.
 #[cfg(feature = "index")]
 pub enum IndexPersistedOrInMemory {

gix/tests/clone/mod.rs

@@ -443,14 +443,19 @@ mod blocking_io {
             "local clone always adopts the name of the remote"
         );
 
-        let short_name = referent.name().shorten();
+        let ref_name = referent.name();
         assert_eq!(
-            repo.branch_remote_name(short_name).expect("remote is set").as_ref(),
+            referent
+                .remote_name(gix::remote::Direction::Fetch)
+                .expect("remote is set")
+                .as_ref(),
             remote_name,
             "the remote branch information is fully configured"
         );
         assert_eq!(
-            repo.branch_remote_ref(short_name).expect("present")?.as_bstr(),
+            repo.branch_remote_ref_name(ref_name, gix::remote::Direction::Fetch)
+                .expect("present")?
+                .as_bstr(),
             "refs/heads/main"
         );
 

gix/tests/fixtures/generated-archives/make_remote_config_repos.tar.xz

@@ -0,0 +1,86 @@
+#!/bin/bash
+set -eu -o pipefail
+
+(mkdir fetch && cd fetch
+  git init -q
+
+  git checkout -b main
+
+  git commit --allow-empty -q -m c1
+
+  git remote add --fetch remote_repo .
+  git branch --set-upstream-to remote_repo/main
+
+  git config branch.broken.merge not_a_valid_merge_ref
+  git config push.default simple
+)
+
+(mkdir push-mapped && cd push-mapped
+  git init -q
+
+  git checkout -b main
+  git commit --allow-empty -q -m c1
+
+  cat<<EOF >.git/config
+[remote "origin"]
+    url = .
+    fetch = +refs/heads/*:refs/remotes/origin/*
+    push = refs/heads/main ; this should be ignored
+    push = refs/heads/main:refs/heads/remapped-main
+    push = refs/heads/main:refs/heads/skipped ; skipped as it's not the first matching one
+    push = refs/heads/feature:refs/heads/remapped-feature ; this is picked up before going to push.default (which would fail)
+
+[branch "main"]
+    remote = "origin"
+    merge = refs/heads/main
+
+[push]
+  default = simple
+
+[branch "feature"]
+    remote = "origin"
+    merge = refs/heads/main  ; this one is remapped to merge from main, which doesn't affect the push remote.
+EOF
+)
+
+(mkdir push-missing && cd push-missing
+  git init -q
+
+  git checkout -b main
+  git commit --allow-empty -q -m c1
+
+  cat<<EOF >.git/config
+[remote "origin"]
+    url = .
+    fetch = +refs/heads/*:refs/remotes/origin/*
+    push = refs/heads/main ; there is a match, but no destination is available
+
+[push]
+  default = current ; this could work, but the default isn't looked at if there are any push specs
+
+[branch "main"]
+    remote = "origin"
+    merge = refs/heads/main
+EOF
+)
+
+(mkdir push-default-current && cd push-default-current
+  git init -q
+
+  git checkout -b main
+  git commit --allow-empty -q -m c1
+
+  cat<<EOF >.git/config
+[remote "origin"]
+    url = .
+    fetch = +refs/heads/*:refs/remotes/origin/*
+
+[push]
+  default = current ; this would be the one setting that works as it ignores 'branch.main.merge'
+
+[branch "main"]
+    remote = "origin"
+    merge = refs/heads/other
+EOF
+)
+