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 support for pushing stacked reviews to GitHub, GitLab, etc. #485

Open
martinvonz opened this issue Aug 22, 2022 · 9 comments
Open

Add support for pushing stacked reviews to GitHub, GitLab, etc. #485

martinvonz opened this issue Aug 22, 2022 · 9 comments

Comments

@martinvonz
Copy link
Member

Description

For users who prefer one review per commit (as we do in this project), it would be useful to have some support for pushing a stack of commits and have it automatically create (or reuse) PRs for each commit. In the case of GitHub, we should also set the base branch appropriately then. I don't know about GitLab or any other providers we would care about.

Maybe we'd want commands like jj github push and jj gitlab push or something like, to make it clear that they're specifically for those providers.

@bklebe
Copy link

bklebe commented Mar 7, 2024

I love this feature in Sapling and would love to see it in Jujutsu as well!

@FirelightFlagboy
Copy link

git town propose have a similar feature where it would create a new pull request for the current branch and setting the base as the configured parent branch.

@steveklabnik
Copy link
Collaborator

See also https://github.com/ejoffe/spr

and @sunshowers's fork, which has experimental jj support https://github.com/sunshowers/spr

@sunshowers
Copy link

Note that spr (and all other stacked diff tooling for GH, afaik) requires that you have push access to the repo you're proposing changes to.

@eopb
Copy link
Contributor

eopb commented Jul 30, 2024

For anyone who stumbles across this issue, I have a little nushell script that gets me some of the way there.

https://github.com/eopb/jj-gh-pr.nu

Even if you don't use nushell, maybe it'll still be useful to get ideas to adapt for your own scripts that you can use until jj has more built in support

@riwsky
Copy link

riwsky commented Aug 6, 2024

in the spirit of @eopb 's comment, here's some bash for submitting stacked PRs to graphite.dev from a jujutsu repo: https://gist.github.com/riwsky/38b17ea3fca70acf20a18c752663aff4

@PhilipMetzger
Copy link
Collaborator

There now are separate issues for both the GitHub (#4555) and Gerrit Integration (#4387). In my opinion this should be kept as a Umbrella task for more integrations.

@arxanas
Copy link
Collaborator

arxanas commented Oct 1, 2024

[topic: one command to handle all forges? replying to @dbarnett's comment in this thread since this is the forge-agnostic thread]

@dbarnett: Yeah, tbh all the integrations I can think of for GitHub would have direct analogs for other forges, and I'm questioning whether it needs anything like an explicit jj github command at all. Besides some slight terminology differences, if you can do it for GH you should probably be able to do it for any other supported remote.

I'm no longer in favor —

Reproducing some messages from the Discord thread here:

@martinvonz: i still feel like a generic command for sending for review or for "merging" a review will be hard to do with a good UX for the CLI. Specifically, it seems like the commands will have to have the union of all options needed by different forges (but probably not Google's internal one, and not including other internal ones)

[...]

@arxanas: How often do people use command-line options to affect the code review? I think most of mine would be either metadata in e.g. the commit message or actions in the web UI
[...]

@PhilipMetzger: iirc, the internal tool has submit and approve, which I both like

@martinvonz: some frequently used options we have with our hg-based solution include for letting the PR get automatically rebased before getting landed, for publishing draft comments (previously entered in the web ui), and for specifying associated bugs [...] you can add additional bug associations when you send for review


[topic: verb choice]

  • git-branchless uses submit to create a code review object,
  • but Google uses "submit" for merging the actual code review:

@martinvonz: we have hg mail for sending for review and hg submit for "merging"

  • I do think "submit" is a little ambiguous and not a great term overall.
  • mail was discussed as a reasonably forge-agnostic option.
    • It doesn't conflict with most forge concepts/terms.
    • I personally think it's a bit unintuitive/undiscoverable.
  • I think upload was discussed as well.
    • Fairly descriptive in my opinion, although it's a little generic.
      • I wonder if we did user research whether it would be found to be most intuitive?
    • Suggests that there would be a corresponding download command.
      • To be fair, "downloading"/getting a local copy of a remote code review is actually a reasonably common workflow.
  • We could reuse push as the "standard" subcommand for this purpose.
    • If this term were adopted:
      • jj git push would create a remote branch.
        • git-branchless calls this case the "branch" forge in its git submit.
      • jj github push/jj gitlab push would actually create pull requests/merge requests respectively.
    • It reuses existing terminology in a reasonably intuitive way.
      • Can reuse pull verb as well... although maybe the semantics of three-way merge mean that we shouldn't use push/pull as the terms?
      • Maybe three-way-merge-ness is a property of the forge? Only the "branch" forge would use it?
        • Maybe that's the difference between push/upload and pull/download?
      • Okay, so maybe this term is not that intuitive?

[topic: standardization and code sharing]

In git-branchless, I haven't been able to abstract over the different forges that much. The current common interface is here:

https://github.com/arxanas/git-branchless/blob/db7cc710658190f83f887adb8c825aaaef014b34/git-branchless-submit/src/lib.rs#L158-L180

In particular, I haven't been able to abstract over topologically submitting updates and setting dependencies, although it might be possible.

  • For efficiency, you often want to batch creates/updates rather than issue 1x serialized command per create/update.
    • But then each forge needs to know how to topologically sort within a batch, and when it's appropriate/necessary to do so?
  • See Github forge problems arxanas/git-branchless#1259 for discussion of certain implementation issues for the GitHub forge.
    • It also motivates a separate "land"/"submit"/"merge"/whatever subcommand, separate from the subcommand that only creates/updates the code review objects.
  • There's a common problem where, when you merge a commit at the bottom of a stack, then rebase dependent commits + code reviews, the newly-rebased code reviews "forget" that they were dependent on the now-merged code reviews.
    • Not sure if we can or want to solve it generically. It probably requires extra storage; both in-band and out-of-band are options.
    • This arose for at least Phabricator and GitHub so far, and I'm sure would continue to arise for future forges.

Ideally we would also reuse certain UI components to make the output formatting consistent.

  • I had been iterating on that for git-branchless.
  • Originally I printed one line per status ("updated"/"already up to date"/"error", etc.).
  • But i think the best UI is one line per commit, with each line including an identifier and a link to to the remote object, if any.
  • Users likely would want to template the output for use in their own scripting.

@aaomidi
Copy link

aaomidi commented Nov 15, 2024

Another possible integration is with aviator.co and their stacked PRs

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

No branches or pull requests

10 participants