git-push: allow setting multiple push remotes - #10014
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
1b47349 to
125dd04
Compare
There was a problem hiding this comment.
this file is best reviewed with "hide whitespace" on.
|
This project does not use conventional commits. Please rename all your subjects with an appropriate topic, such as "git:". https://docs.jj-vcs.dev/latest/contributing/#commit-guidelines |
125dd04 to
b277496
Compare
josephlou5
left a comment
There was a problem hiding this comment.
Per https://docs.jj-vcs.dev/latest/contributing/#commit-guidelines, please squash relevant commits together; each commit should be standalone. This means each commit should pass all tests, docs and config schema updates should happen in the commit that changed the processing logic, and you can probably squash all test commits together.
Also, maybe change the PR title with the updated topic as well.
| } | ||
| Ok(remote) | ||
| Ok(StringExpression::exact(remote)) | ||
| } else { |
There was a problem hiding this comment.
Does this mean if multiple remotes are configured in the repo (and git.push is not set), then still only DEFAULT_REMOTE = 'origin' is pushed to?
There was a problem hiding this comment.
if multiple remotes are configured, the result of get_single_remote is used. AFAICT that is unchanged behavior.
If you refer to setting multiple push remotes in git, it doesn't look like that gets picked up presently at all.
| #[command(group(ArgGroup::new("what").conflicts_with("specific")))] | ||
| pub struct GitPushArgs { | ||
| /// The remote to push to (only named remotes are supported) | ||
| /// The remote to push to (only named remotes are supported, can be |
There was a problem hiding this comment.
Nit: I would put "can be repeated" in separate parentheses.
There was a problem hiding this comment.
This was copied from fetch, do you feel strongly enough to change this there as well?
| #[arg(long)] | ||
| /// | ||
| /// By default, the specified pattern matches remote names with glob syntax, | ||
| /// e.g. `--remote '*'`. You can also use other [string pattern syntax]. |
There was a problem hiding this comment.
Pedantic nit: "e.g." should have a comma after it.
There was a problem hiding this comment.
This was copied from fetch, do you feel strongly enough to change this there as well?
| }, | ||
| Ok(None) => {} | ||
| Err(reason) => reason.print(ui)?, | ||
| for remote in matching_remotes { |
There was a problem hiding this comment.
Can we extract smaller functions that operate on a remote? For example, it might make sense to extract the following block (and similar) using a |targets| -> bool callback.
jj/cli/src/commands/git/push.rs
Lines 296 to 319 in 253d3be
There was a problem hiding this comment.
Could you be more specific what you mean by |targets| -> bool callback?
As I mentioned in the PR desc, I considered breaking the function apart a bit by moving the branches into functions outside, e.g.
diff --git a/cli/src/commands/git/push.rs b/cli/src/commands/git/push.rs
index 0dc219ecfc..a1d4983b29 100644
--- a/cli/src/commands/git/push.rs
+++ b/cli/src/commands/git/push.rs
@@ -320,36 +320,8 @@
let view = tx.repo().view();
if args.all {
for remote in matching_remotes {
- let mut ref_updates = GitPushRefTargets::default();
-
- let mut commits_validator =
- CommitsValidator::new(ui, tx.base_workspace_helper(), remote, args)?;
- for (name, targets) in view.local_remote_bookmarks(remote) {
- [...]
- }
- for (name, targets) in view.local_remote_tags(remote) {
- [...]
- }
+ let ref_updates =
+ classify_tags_and_bookmark_updates(ui, args, &tx, view, remote).await?;
let tx_description = format!(
"{TX_DESC_PUSH}all bookmarks/tags to git remote {remote}",
remote = remote.as_symbol()
@@ -683,6 +655,45 @@
Ok(())
}
+async fn classify_tags_and_bookmark_updates(
+ ui: &mut Ui,
+ args: &GitPushArgs,
+ tx: &WorkspaceCommandTransaction<'_>,
+ view: &View,
+ remote: &RemoteName,
+) -> Result<GitPushRefTargets, CommandError> {
+ let mut ref_updates = GitPushRefTargets::default();
+ let mut commits_validator =
+ CommitsValidator::new(ui, tx.base_workspace_helper(), remote, args)?;
+ for (name, targets) in view.local_remote_bookmarks(remote) {
+ [...]
+ }
+ for (name, targets) in view.local_remote_tags(remote) {
+ [...]
+ }
+ Ok(ref_updates)
+}
+
#[derive(Clone, Debug)]
struct RejectedCommitReason {
commit: Commit,
Notably this is not a callback, nor has a remotely similar signature.
There was a problem hiding this comment.
I mean you can probably parameterize a filtering predicate:
|targets| !targets.remote_ref.is_tracked()
There was a problem hiding this comment.
I'm not sure if I'm tracking exactly, but i do see the exact repetition in at least 3 of the four branches. Ie. the extracted function i proposed above can be more generic to apply to all three of these places
#[derive(Debug, Copy, Clone)]
struct ClassifyParams {
predicate: fn(LocalAndRemoteRef) -> bool,
allow_new: bool,
allow_delete: bool,
}
async fn classify_tags_and_bookmark_updates(
remote: &RemoteName,
params: ClassifyParams,
ui: &mut Ui,
args: &GitPushArgs,
tx: &WorkspaceCommandTransaction<'_>,
view: &View,
) -> Result<GitPushRefTargets, CommandError> {
let mut ref_updates = GitPushRefTargets::default();
let mut commits_validator =
CommitsValidator::new(ui, tx.base_workspace_helper(), remote, args)?;
for (name, targets) in view
.local_remote_bookmarks(remote)
.filter(|(_, targets)| (params.predicate)(*targets))
{
[...]
}
for (name, targets) in view
.local_remote_tags(remote)
.filter(|(_name, targets)| (params.predicate)(*targets))
{
[...]
}
Ok(ref_updates)
}which turns all call sites into
let params = ClassifyParams {
// classify all tags and bookmarks (implied by --all)
predicate: |_targets| true,
// implied by --all
allow_new: true,
allow_delete: args.deleted,
};
let ref_updates =
classify_tags_and_bookmark_updates(remote, params, ui, args, &tx, view).await?;Does that align better with what you had in mind?
There was a problem hiding this comment.
Yeah, something like that. I'd use impl FnMut or impl Fn instead of a function pointer.
There was a problem hiding this comment.
Yeah impl Fn works of course.
Added this as a separate refactor.
There is one (2?) more place(s) with a similar pattern in the else branch, but unlike the --{all,deleted,tracked} modes there are subtle differences between the filters between tags and bookmarks. Specifically, the filter mutates seen_{bookmarks,tags}. I'm not quite convinced that passing multiple filters, and adding name to each filter is worth capturing this case in the abstraction.
FWIW I attempted this as well, besides the noise, its also fails to match up the lifetimes of name with seen_* when attempting to add it within the filter function (which still seems like an odd practice in general).
Diff of splitting bookmark and tag predicate
diff --git a/cli/src/commands/git/push.rs b/cli/src/commands/git/push.rs
index e94731a126..70f0d6463d 100644
--- a/cli/src/commands/git/push.rs
+++ b/cli/src/commands/git/push.rs
@@ -319,16 +319,17 @@
let view = tx.repo().view();
if args.all {
- let params = ClassifyParams {
+ let mut params = ClassifyParams {
// classify all tags and bookmarks (implied by --all)
- predicate: |_targets: LocalAndRemoteRef| true,
+ bookmark_predicate: |_: &RefName, _targets: LocalAndRemoteRef| true,
+ tag_predicate: |_: &RefName, _targets: LocalAndRemoteRef| true,
// implied by --all
allow_new: true,
allow_delete: args.deleted,
};
for remote in matching_remotes {
let ref_updates =
- classify_tags_and_bookmark_updates(remote, ¶ms, ui, args, &tx, view).await?;
+ classify_tags_and_bookmark_updates(remote, params, ui, args, &tx, view).await?;
let tx_description = format!(
"{TX_DESC_PUSH}all bookmarks/tags to git remote {remote}",
remote = remote.as_symbol()
@@ -336,16 +337,21 @@
by_remote.push((remote, ref_updates, tx_description));
}
} else if args.tracked {
- let params = ClassifyParams {
+ let mut params = ClassifyParams {
// classify tracked tags and bookmarks only
- predicate: |targets: LocalAndRemoteRef| targets.remote_ref.is_tracked(),
+ bookmark_predicate: |_: &RefName, targets: LocalAndRemoteRef| {
+ targets.remote_ref.is_tracked()
+ },
+ tag_predicate: |_: &RefName, targets: LocalAndRemoteRef| {
+ targets.remote_ref.is_tracked()
+ },
// doesn't matter
allow_new: true,
allow_delete: args.deleted,
};
for remote in matching_remotes {
let ref_updates =
- classify_tags_and_bookmark_updates(remote, ¶ms, ui, args, &tx, view).await?;
+ classify_tags_and_bookmark_updates(remote, params, ui, args, &tx, view).await?;
let tx_description = format!(
"{TX_DESC_PUSH}all tracked bookmarks/tags to git remote {remote}",
@@ -354,9 +360,14 @@
by_remote.push((remote, ref_updates, tx_description));
}
} else if args.deleted {
- let params = ClassifyParams {
+ let mut params = ClassifyParams {
// classify tags and bookmarks that are not (no longer) present locally
- predicate: |targets: LocalAndRemoteRef| !targets.local_target.is_present(),
+ bookmark_predicate: |_: &RefName, targets: LocalAndRemoteRef| {
+ !targets.local_target.is_present()
+ },
+ tag_predicate: |_: &RefName, targets: LocalAndRemoteRef| {
+ !targets.local_target.is_present()
+ },
// doesn't matter
allow_new: true,
allow_delete: true,
@@ -365,7 +376,7 @@
for remote in matching_remotes {
// There shouldn't be new heads to push, but we run validation for consistency.
let ref_updates =
- classify_tags_and_bookmark_updates(remote, ¶ms, ui, args, &tx, view).await?;
+ classify_tags_and_bookmark_updates(remote, params, ui, args, &tx, view).await?;
let tx_description = format!(
"{TX_DESC_PUSH}all deleted bookmarks/tags to git remote {remote}",
remote = remote.as_symbol()
@@ -485,41 +496,26 @@
} else {
find_target_revisions(ui, tx.base_workspace_helper(), &args.revisions).await?
};
- for (name, targets) in tx.base_repo().view().local_remote_bookmarks(remote) {
- if !matches_local_target(targets, &target_revisions) || !seen_bookmarks.insert(name)
- {
- continue;
- }
- let remote_symbol = name.to_remote_symbol(remote);
- let allow_new = false;
- let allow_delete = false;
- match classify_bookmark_update(remote_symbol, targets, allow_new, allow_delete) {
- Ok(Some(update)) => match commits_validator.validate_update(&update).await? {
- Ok(()) => ref_updates.bookmarks.push((name.to_owned(), update)),
- Err(reason) => {
- reason.print_bookmark(ui, tx.base_workspace_helper(), name)?;
- }
- },
- Ok(None) => {}
- Err(reason) => reason.print(ui)?,
- }
- }
- for (name, targets) in tx.base_repo().view().local_remote_tags(remote) {
- if !matches_local_target(targets, &target_revisions) || !seen_tags.insert(name) {
- continue;
- }
- let remote_symbol = name.to_remote_symbol(remote);
- let allow_new = false;
- let allow_delete = false;
- match classify_tag_update(remote_symbol, targets, allow_new, allow_delete) {
- Ok(Some(update)) => match commits_validator.validate_update(&update).await? {
- Ok(()) => ref_updates.tags.push((name.to_owned(), update)),
- Err(reason) => reason.print_tag(ui, tx.base_workspace_helper(), name)?,
- },
- Ok(None) => {}
- Err(reason) => reason.print(ui)?,
- }
- }
+
+ let params = ClassifyParams {
+ bookmark_predicate: |name: &RefName, targets: LocalAndRemoteRef| {
+ matches_local_target(targets, &target_revisions) && seen_bookmarks.insert(name)
+ },
+
+ tag_predicate: |name: &RefName, targets: LocalAndRemoteRef| {
+ matches_local_target(targets, &target_revisions) && seen_tags.insert(name)
+ },
+
+ allow_new: false,
+ allow_delete: false,
+ };
+
+ let remaining_ref_updates =
+ classify_tags_and_bookmark_updates(remote, params, ui, args, &tx, view).await?;
+ ref_updates
+ .bookmarks
+ .extend(remaining_ref_updates.bookmarks);
+ ref_updates.tags.extend(remaining_ref_updates.tags);
let tx_description = format!(
"{TX_DESC_PUSH}{names} to git remote {remote}",
@@ -609,26 +605,31 @@
}
#[derive(Debug, Copy, Clone)]
-struct ClassifyParams<F> {
- predicate: F,
+struct ClassifyParams<F, G> {
+ bookmark_predicate: F,
+ tag_predicate: G,
allow_new: bool,
allow_delete: bool,
}
-async fn classify_tags_and_bookmark_updates<F: Fn(LocalAndRemoteRef) -> bool>(
+async fn classify_tags_and_bookmark_updates<F, G>(
remote: &RemoteName,
- params: &ClassifyParams<F>,
+ mut params: ClassifyParams<F, G>,
ui: &mut Ui,
args: &GitPushArgs,
tx: &WorkspaceCommandTransaction<'_>,
view: &View,
-) -> Result<GitPushRefTargets, CommandError> {
+) -> Result<GitPushRefTargets, CommandError>
+where
+ F: FnMut(&RefName, LocalAndRemoteRef) -> bool,
+ G: FnMut(&RefName, LocalAndRemoteRef) -> bool,
+{
let mut ref_updates = GitPushRefTargets::default();
let mut commits_validator =
CommitsValidator::new(ui, tx.base_workspace_helper(), remote, args)?;
for (name, targets) in view
.local_remote_bookmarks(remote)
- .filter(|(_, targets)| (params.predicate)(*targets))
+ .filter(|(name, targets)| (params.bookmark_predicate)(name, *targets))
{
let remote_symbol = name.to_remote_symbol(remote);
match classify_bookmark_update(
@@ -649,7 +650,7 @@
}
for (name, targets) in view
.local_remote_tags(remote)
- .filter(|(_name, targets)| (params.predicate)(*targets))
+ .filter(|(name, targets)| (params.tag_predicate)(name, *targets))
{
let remote_symbol = name.to_remote_symbol(remote);
match classify_tag_update(
There was a problem hiding this comment.
@yuja i rebased this and incorporated your description changes, at least in spirit.
There was a problem hiding this comment.
Can you move the refactoring patches to the beginning? You'll also need to squash the doc and test changes into the implementation patch. Please also clean up the commit messages.
https://docs.jj-vcs.dev/latest/contributing/
Oh, and you'll need to sign the Google CLA. Some of us aren't allowed to review PRs without one.
There was a problem hiding this comment.
Thanks, I the refactor ahead, and squashed impl, docs, and tests. Updated the description of the commit as well (reduced duplication from earlier squashes and added some context).
Signed the CLA yesterday, but it didn't get updated here, should be fixed now.
I pushed back a bit to your suggestions reg. the ClassifyParams type and arg. Happy to change those if you indeed feel strongly about them, but would otherwise leave them as is.
b277496 to
f8d8182
Compare
63f1b66 to
cd6ee8b
Compare
| predicate: F, | ||
| allow_new: bool, | ||
| allow_delete: bool, | ||
| } |
There was a problem hiding this comment.
nit: This type can be passed into classify_*_update() by removing predicate: F.
There was a problem hiding this comment.
originally, I just passed all three as args to classify_tags_and_bookmark_updates but that appeared to take the method past the configured maximum number of arguments.
My intention to break out this type as is was to bundle the case specific params together.
There was a problem hiding this comment.
An impl Fn closure is often passed as a standalone argument because its concrete type cannot be named. Including predicate: F in this type doesn't seem to improve readability. In addition, we can replace positional bool, bool arguments with this type.
| remote: &RemoteName, | ||
| params: &ClassifyParams<F>, | ||
| ui: &mut Ui, | ||
| args: &GitPushArgs, |
There was a problem hiding this comment.
nit: Since ClassifyParams is resolved by the caller, it also seems better to resolve CommitsValidator there and remove args: &GitPushArgs.
There was a problem hiding this comment.
hm, my thinking was that the implementation and details of classifying remains in the new function. Especially since the validator is a) setup identically, and b) not used beyond the function.
There was a problem hiding this comment.
GitPushArgs includes various parameters, and some of them overlap with ClassifyParams { allow_new: args.all, allow_delete: args.deleted }. Since this function takes precise parameters, it's better to avoid passing unused junk. Does that make sense?
| }, | ||
| Ok(None) => {} | ||
| Err(reason) => reason.print(ui)?, | ||
| for remote in matching_remotes { |
There was a problem hiding this comment.
Can you move the refactoring patches to the beginning? You'll also need to squash the doc and test changes into the implementation patch. Please also clean up the commit messages.
https://docs.jj-vcs.dev/latest/contributing/
Oh, and you'll need to sign the Google CLA. Some of us aren't allowed to review PRs without one.
cd6ee8b to
d7ae236
Compare
All handlers for `--all`, `--deleted`, and `--tracked` follow the same pattern: * create a `CommitValidator` * classify bookmark updates: * get all local bookmarks * skip _excluded_ targets * classify update to the bookmark and push that update to `ref_updates` * classify tag updates (same as bookmarks but acting on tags) The handlers only differ in the predicate used to filter bookmarks/tags and whether classification allows creation/deletion of bookmarks. Therefore its possible to extract the collection of bookmark/tag changes, and pass it the filter, and creation/deletion bits.
Remotes can now be configured via `git.push` set to a string pattern or array of string patterns, or with the repeatable --remote flag, which also accepts string patterns. Brought up by jj-vcs#7833; while `jj git fetch` allows fetching from multiple remotes at the same time, `jj git push` is limited to one remote. Closes jj-vcs#7833
d7ae236 to
9ee1ece
Compare
| remote: &RemoteName, | ||
| params: &ClassifyParams<F>, | ||
| ui: &mut Ui, | ||
| args: &GitPushArgs, |
There was a problem hiding this comment.
GitPushArgs includes various parameters, and some of them overlap with ClassifyParams { allow_new: args.all, allow_delete: args.deleted }. Since this function takes precise parameters, it's better to avoid passing unused junk. Does that make sense?
| async fn classify_tags_and_bookmark_updates<F: Fn(LocalAndRemoteRef) -> bool>( | ||
| remote: &RemoteName, | ||
| params: &ClassifyParams<F>, | ||
| ui: &mut Ui, |
There was a problem hiding this comment.
nit: ui is usually the first argument, and we don't need &mut.
| predicate: F, | ||
| allow_new: bool, | ||
| allow_delete: bool, | ||
| } |
There was a problem hiding this comment.
An impl Fn closure is often passed as a standalone argument because its concrete type cannot be named. Including predicate: F in this type doesn't seem to improve readability. In addition, we can replace positional bool, bool arguments with this type.
Addresses #7833
That is it mirrors the
giit fetchsubcommand's config, extending thegit.pushsetting to accept lists of remotes or string patterns describing remotes to push to.Additionally, allows users to supply the
--remoteflag multiple times, also with remote names or patterns.First PR here, happy to work on the granularity of changes or wording/docuemntation that doesnt fit the project.
I tried to keep the overall change minimal, but I did notice a tendency to long monolithic files and functions. Is that "by design" or would there be an interest in breaking some of that up into more fine grained operations? (speaking of e.g factoring out hte validation from the execution (loops) as well as argument / mode handling).
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)how it works, how it's organized), including any code drafted by an LLM.
an eye towards deleting anything that is irrelevant, clarifying anything
that is confusing, and adding details that are relevant. This includes,
for example, commit descriptions, PR descriptions, and code comments.