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

Add jj git push --stack to push all working branches #1415

Open
rslabbert opened this issue Mar 23, 2023 · 9 comments
Open

Add jj git push --stack to push all working branches #1415

rslabbert opened this issue Mar 23, 2023 · 9 comments
Labels
enhancement New feature or request polish🪒🐃 Make existing features more convenient and more consistent

Comments

@rslabbert
Copy link
Contributor

Description

A typical workflow for me is to create some commits in a stack, and jj git push --change @ them to make a couple of PRs on Github. When making changes, every commit that has changed will need a jj edit <ref> && jj git push to get it up to date, which can become quite cumbersome when editing commit 1 of x. Alternatively I can build up a jj git push --branch ... --branch ... --branch ... command as I go but that feels suboptimal.

Barring a more comprehensive approach for handling local/remote interaction (see #1039 and #1136), a good short/medium term solution in my opinion would be a --stack option for jj git push which can would push every branch of the form push-* up and down from the starting ref @ until it hits a branch that doesn't follow that form.

Open questions

  1. Using the push-* branch names for deciding what to do would be a convenient way to avoid having to know what the "main" branch is for any given repository, but is it worth just pulling in the branch detection and configuration logic from git-branchless and building a solid foundation for sync first? It should be easy to migrate to it later.
  2. Should you be able to provide a -r flag to jj git push --stack to specify which ref to start at? I don't think that would make sense since technically the -r would be on jj git push which doesn't make sense currently. Maybe --stack should always have a ref passed like --stack @?
  3. Are there any situations where this would cause obvious issues such as accidentally pushing wrong branches?
  4. Should there be a default prompt to continue before we push multiple branches? I'm thinking --all or --stack due to their potential for unexpected consequences should require a prompt first or --no-prompt?
  5. Should it be called something other than jj git push --stack? git-branchless uses git submit, Sapling uses sl pr submit, and graphite uses gt stack submit.
@rslabbert rslabbert added enhancement New feature or request polish🪒🐃 Make existing features more convenient and more consistent labels Mar 23, 2023
@martinvonz
Copy link
Member

I'd like a form of jj git push that takes a revset and pushes all branches pointing to any commit in the set. Let's call it jj git push --branch-targets <revset> for now (there's probably a better name). Then jj git push --stack might be jj git push --branch-targets '(remote_branches()..@):'.

This is also related to the idea of defining a configurable main branch like git-branchless does. Then we might have a default revset alias stack defined as (<configured main branch>..@):, so you could do jj git push --branch-targets stack.

#485 is related.

@arxanas
Copy link
Contributor

arxanas commented Mar 23, 2023

Re @rslabbert's points:

  1. I didn't understand this (possibly I'm not used to the jj workflow). What does auto-naming push-* branches have to do with knowing the main branch for the repository? What is it that you would do differently if you knew the main branch?
  2. Like @martinvonz says, and as git-branchless does, I vote submitting all branches that point into a revset.
  3. The failure case I had in mind is accidentally (force-!)pushing a long-lived branch, such as main. Some Git hosting providers let you "protect" those branches on the server-side, but I think it would be reasonable to provide protections on the client-side as well.
  4. So far, git-branchless does not prompt for multiple branches. Instead, the behavior is inverted from how @martinvonz describes it (IIUC): only existing branches are pushed by default, and new branches are pushed/generated by passing the --create flag. (So far: the default backend will not generate branch names and will not push unbranched commits; the GitHub backend will one day generate a branch name if none exists and push it; the Phabricator backend generates a branch name corresponding to the change ID – but probably shouldn't, because the change ID is embedded in the commit message already – and then submits it.) Also, the default revset is stack(), which naturally excludes the main branch, so that you don't push it by accident when running git submit without arguments.
  5. I think the "submit" name implies that it does a little more than just push refs, like handle dependencies.

@rslabbert
Copy link
Contributor Author

@arxanas:

  1. The auto-naming is a convenient "hack" for having a rough approximation for what is a jj created branch (via jj git push --change @), but it's still a hack. It doesn't account for branches that don't exist yet (though it handles cases where you might have one branch to have multiple commits) and is very unlikely to conflict with base branches (meaning if you're building off main/master/release/etc. it will stop once it gets to that point). If we knew what the main branch was, however, we could handle all these cases more correctly, including pushing new commits to new branches.

Plan going forward then so far I think would be (names are placeholders, see questions below for naming):

  1. Add support for marking the main branch of a repository.
  2. Add a revset function (thoughts on trunk()?) that points to the main branch.
  3. Add a flag to jj init called --trunk/--main/something else that adds the main branch on init, otherwise port the main branch detection logic from git-branchless (code here) and prompt the user. This would require a friendly error message on upgrade if the setting doesn't exist.
  4. (Alternative to/in addition to point 3. above) don't prompt the user for the main branch until we need it, i.e. when the trunk() revset is used.
  5. Add a default stack() revset alias that resolves to (trunk()..@):
  6. Add a -r/--revisions option to jj git push. This will operate similar to --branch or --change (see question 3. below) but will operate on the entire revset.
  7. (Future) extend jj git push to support github/gitlab/etc. PR creation. Should require a config option (as it will most likely require auth tokens). This could eventually make its way into a submit command instead.

Questions:

  1. Name for the revset function trunk/main/etc.?
  2. Name of the argument to jj git push? -r/--revisions seems to make sense for consistency.
  3. Does using the stacked push create branches similar to --change, or does it only push branches that already exist? If so, this effectively supersedes the --change flag. Should --change maybe be extended to take a revset instead? Change IDs should stay constant so adjusting --change instead might make sense? It might be nice to provide a shortened version of it though.

@martinvonz
Copy link
Member

That all sounds good to me. Thank you!

For 3 vs 4, I'm leaning towards 4, so we don't force users to have a configured main branch for small repos like for tracking dotfiles.

I don't have much opinion about trunk() vs main(), but I'm leaning towards trunk() to reduce confusion between the typical main branch name and the revset name.

-r/--revisions sounds good to me. I think we should not make it create new branches but only push branches that already pointed to the given revisions. And yes, I think we should extend --change to take a revset, maybe renaming it to plural --changes/-c.

Having the configured main branch also enables a sync command, combining pull/fetch and rebase.

@arxanas
Copy link
Contributor

arxanas commented Mar 25, 2023

Add a revset function (thoughts on trunk()?) that points to the main branch.

git-branchless calls this main().

Add a default stack() revset alias that resolves to (trunk()..@):

In git-branchless, stack() also includes descendant commits of @, which I think is quite useful in practice. Actually, the definition is something like (trunk()..@):, so it's possible for a "cousin" commit to be included in the stack as well. EDIT: this is exactly the same revset, I can't read

@rslabbert
Copy link
Contributor Author

jj git push -r is supported as of 7f3d07e, which means that right now I'm able to use the following config to manage this use case for the two differently named trunk branches I have to deal with frequently:

# DISCLAIMER: sync and push work for me but be careful using it if you're not 100%
[aliases]
sync = ["rebase", "-s", "roots((trunk..@) ~ empty())", "-d", "trunk@origin"]
sl = ["log", "-r", "(trunk..@): | (trunk..@)-"]
push = ["git", "push", "-r", "((trunk..@): & remote_branches()) & ~author(me)"]

[revset-aliases]
trunk = "latest((present(main) | present(master)) & remote_branches())"

A few notes:

  1. ~ empty() in sync is used to work around Drop commits that were made empty by rebase #229
  2. We can't have multiple commands in a single alias, so jj git fetch && jj sync are still two different calls.
  3. jj git fetch doesn't support revsets, so I unfortunately we can't use jj git fetch -r trunk, which is a limitation with this solution at the moment as it ends up fetching all branches when fetching (though somewhat resolved by setting git.auto-local-branch = false).
  4. The trunk alias is a bit wonky, using remote_branches() to ensure we're definitely using the one that actually exists, and latest to ensure we only resolve to a single (with the assumption that the latest one is actually the trunk) but it works pretty consistently on standard repos.

@martinvonz
Copy link
Member

3. jj git fetch doesn't support revsets, so I unfortunately we can't use jj git fetch -r trunk

There's jj git fetch --branch trunk since #1146.

push = ["git", "push", "-r", "((trunk..@): & remote_branches()) & ~author(me)"]

A few things:

  • Will jj git push -r 'remote_branches()..@' not do what you want? Why not?
  • Why the & ~author(me) bit there? You don't want to push your own branches? Or am I just not reading that right?
  • There's a mine() revset since revset: add function mine() #2091

@rslabbert
Copy link
Contributor Author

Note: Covered some of these thoughts in #2088 (comment) as well.

  1. I've been hesitant to use remote_branches() in a default command since it could easily include a branch I don't intend. I personally would prefer these default to fail closed instead of being too helpful. Additionally, once a branch is pushed remote_branches()..@ will stop working. Switching to ``remote_branches()::@` will work but I'd need to run through the options.
  2. Adding remote_branches() in as an intersection was meant to serve two purposes:
  3. Ensure we're only trying to push to branch that already exist remotely (generally created beforehand, for example I use a submit alias which is git push --change @; and
  4. Handle the case where a stack consists of single and multi-commit PRs. Sometimes having each commit as a separate PR isn't worthwhile and PR 4 or a 7 stack might have 2 commits. I added without the & remote_branches() to ensure it only include the branches in that push, though that might be redundant and already expected.
  5. The & ~author(me) was supposed to be & author(me), I think I might have just done something silly.
  6. It might be good to include mine() in some capacity with the default push command and that can definitely be added in as & mine() in my aliases.

@martinvonz
Copy link
Member

  1. Additionally, once a branch is pushed remote_branches()..@ will stop working.

The remote branch will still point to the old commit then (the one you pushed last time), so it should work. I.e. your local branch will have forked from the remote branch and will thus not be a descendant of the remote branch. Does that make sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request polish🪒🐃 Make existing features more convenient and more consistent
Projects
None yet
Development

No branches or pull requests

3 participants