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

cli: change the default revsets for push to be based on trunk() #2263

Closed
wants to merge 1 commit into from

Conversation

rslabbert
Copy link
Contributor

@rslabbert rslabbert commented Sep 18, 2023

As with #2088, I'm not wedded to this, but it seems like a valuable change for my workflow at least and matches git-branchless submit.

Builds on the trunk alias from #2088. The benefits of this over the previous default in my opinion is:

  1. It captures the entire stack range instead of just up to @, meaning you can run it from inside a stack; and
  2. It's built on trunk()..@, so if the trunk branch was accidentally modified, the default push won't include it.

Note: Once trunk() is made into a built-in function, we can create a stack() function that resolves to (trunk()..@):: which should be a bit cleaner than the current approach.

Copy link
Contributor

@ilyagr ilyagr left a comment

Choose a reason for hiding this comment

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

I'm not sure whether this is always an improvement.

The situation this is meant for, as far as I understand, is if a descendant of @ or of @- has branches on it. Currently, jj git push wouldn't push those, which may or may not be intuitive.

One argument for the current behavior is that jj co <some-rev>; jj git push allows for a somewhat surgical pushing, if the work on the descendants is too incomplete to share.

Another possible problem with the approach of this PR is if a tree has a huge side branch (* represent commits, the root of the tree is on the left):

-----*------------------------* (trunk)
      \
       ----*---*---*----*------* @
            \
             --*--*----* (other stuff)
                \  \
                 *  *

With this PR, jj git push would push all the branches of (other stuff) as well. I'm not sure how common such a situation is, but it feels mildly worrying.

OTOH, I might not be the best person to judge this since I almost always use jj git push --all.

@@ -126,7 +126,7 @@ pub struct GitCloneArgs {
/// Push to a Git remote
///
/// By default, pushes any branches pointing to
/// `remote_branches(remote=<remote>)..@`. Use `--branch` to push specific
/// `(trunk()..@):: & branches()`. Use `--branch` to push specific
Copy link
Contributor

Choose a reason for hiding this comment

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

The & branches() seems to be unnecessary because of how jj git push -r is defined.

@ilyagr
Copy link
Contributor

ilyagr commented Sep 19, 2023

OTOH, an argument for something like this is that then the default "push"-set will match the default log revset.

@rslabbert
Copy link
Contributor Author

Tabling this until immutable() is in place and maybe we have drafts()/stack(x).

Either way, I think a good approach would be to ensure that the default log, push, and rebase are all in sync. Especially if we want to draw a distinction between rebase vs merge workflows?

For example, if we're in rebase/stack world, given a stack with 2 commits on main if this is the default log:

@ commit-2 push-2*
|
◉ commit-1 push-1*
|
◉ commit-0 main

I suspect we'd want the default push to be push-1 and push-2 and the default rebase destination to be main.

@rslabbert rslabbert closed this Sep 25, 2023
@rslabbert rslabbert deleted the push-tkvnxuwwuzsq branch September 25, 2023 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants