Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Push with refspec #2542

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

vlad-anger
Copy link

@vlad-anger vlad-anger commented Mar 1, 2025

Fixes #2537

git2 merged necessary PR

This PR is_blocked until new version git2 lib released
For now use git2 from master to be able to test PR.


Assume git config:

[branch "oblava"]
	remote = origin
	merge = refs/heads/master
[push]
  default = upstream

Prev. behavior:
push -> push oblava -> remote oblava <- new remote created

New behavior:
push -> push oblava -> remote master


I followed the checklist:

  • I added unittests
  • I ran make check without errors (see commit with tests fixes)
  • I tested the overall application
  • I added an appropriate item to the changelog

Copy link
Contributor

@naseschwarz naseschwarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for completeness, I'll add a reference to our discussion @vlad-anger. Thank you for the productive discussion!

In #2537 we discussed that the new behavior should be conditional on the value of push.default: Only if push.default == upstream, merge.*.branch is used as the remote branch.

@cruessler
Copy link
Collaborator

cruessler commented Mar 15, 2025

Just to make sure I get the context right: does any of this require changes to this part of the codebase?

/// 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
/// >
/// > The remote to push to by default. Overrides `branch.<name>.remote` for all branches, and is
/// > overridden by `branch.<name>.pushRemote` for specific branches.
///
/// > branch.<name>.remote
/// >
/// > When on branch `<name>`, it tells `git fetch` and `git push` which remote to fetch from or
/// > push to. The remote to push to may be overridden with `remote.pushDefault` (for all
/// > branches). The remote to push to, for the current branch, may be further overridden by
/// > `branch.<name>.pushRemote`. 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
/// > and `remote.pushDefault` for pushing.
///
/// [git-config-remote-push-default]: https://git-scm.com/docs/git-config#Documentation/git-config.txt-remotepushDefault
/// [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_push(
repo_path: &RepoPath,
) -> Result<String> {
let repo = repo(repo_path)?;
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> {
scope_time!("get_default_remote_for_push_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.{}.pushRemote", &remote_name);
if let Ok(entry) = config.get_entry(&entry_name) {
return bytes2string(entry.value_bytes());
}
if let Ok(entry) = config.get_entry("remote.pushDefault") {
return bytes2string(entry.value_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)
}

Or is that not the case as this PR deals with branches while the code I linked to deals with remotes?

@vlad-anger
Copy link
Author

Or is that not the case as this PR deals with branches while the code I linked to deals with remotes?

Yeah those helpers out of scope, they tell which remote can be used,
while this PR is fixing different local/remote branches issue,

Ex. locally i have branch t1, which should be pushed to remote t2.
To achieve that, you could have push.default=upstream &
branch.t1.merge=refs/heads/t2
For now gitui will try to push t1 -> t1 always

@vlad-anger
Copy link
Author

Tweaked logic, updated PR desc.
Waiting for upstream version release, so CI build won't fail and we
can do final checks

Copy link
Contributor

@naseschwarz naseschwarz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for addressing my concerns. 👍

Here are some thoughts on the current version.

Ok(PushDefaultStrategyConfig::try_from(
entry_str.as_str(),
)
.unwrap_or_default())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: The behavior of the git command line client is to reject a push if the value of push.default is defined but not valid.

% git -c push.default=snerflop push origin HEAD
error: malformed value for push.default: snerflop
error: must be one of nothing, matching, simple, upstream or current
fatal: unable to parse 'push.default' from command-line config
128 %

Doing this might come in handy in case git adds new options in the future, which gitui does not support, so we don't accidentally implement incorrect push behavior.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Now we'll fall with normal err
image

@naseschwarz
Copy link
Contributor

naseschwarz commented Mar 17, 2025

Apart from the review item regarding pushing tags, I believe the push logic in this version is correctly aligned with git for push.default in {upstream, simple}. ✔️

matching and current are options that, as far as I can tell, fall back to simple in case a push source is given, which I assume is always the case in push_raw (unless delete is true, in which case only a destination is used). Thus, I'd agree that the implementation covers these correctly, too. It might be worth making this explicit so that devs keep this in mind when working on push_raw, e.g. by actually explicitly writing match on push_default_strategy and adding a short remark in the branch that does nothing.

@vlad-anger
Copy link
Author

@naseschwarz I liked first review round, let's go for another one!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure to follow config push settings
4 participants