Monorepo for Tangled tangled.org
854
fork

Configure Feed

Select the types of activity you want to include in your feed.

proposal: pull requests #311

open opened by anirudh.fi

As most of you probably know, we're working on a pull requests implementation for Tangled. The primary goal isn't to simply reimplement what GitHub has done for well over a decade—but to improve on the status quo, while making sure there's still familiarity to the whole flow. Keeping that in mind, we're introducing two kinds of pull requests.

  1. PRs v1 (patch requests)
  2. PRs v2 (submissions)

pull requests v1 (patch requests)#

This is the first version of pull requests that we will launch. This follows a very simple model and serves to encourage smaller, "driveby" contributions. Here's how it would work:

  1. Clone the repository you'd like to contribute to.
  2. Make your change and commit it. You can do this on the main branch.
  3. Run git format-patch -1 HEAD --stdout. You may either pipe this to your clipboard program (pbcopy, wl-clip, ...) or simply copy the output from your terminal.
  4. Head to the repository on Tangled and create a new pull request from the "pulls" tab.
  5. Fill out your title and description of your change, and paste the git format-patch output in the provided textbox.
  6. Submit, and edit later as per review.

Optionally, this can be extended to more than 1 commit (pass in -2, -3, and so on) and paste the resulting patch series into the textbox. This is of course, a very simple model that only facilitates smaller changes that can typically fit within a few small commits. However, we see this as a driver for contributions that probably would not have happened in the traditional flow simply due to the overhead involved.

pull requests v2 (submissions)#

This workflow blends the more familiar GitHub PR model with changeset-oriented reviewing by allowing developers to work in feature branches and submit consolidated changes for review. This is what it would look like:

  1. branch-based development
    • Developer creates and pushes to a feature branch as their working area
    • Multiple incremental commits can be made during development
  2. PR Submission
    • When ready for review, developer initiates a "PR Submission"
    • System automatically squashes all branch commits into a single commit
    • Developer is prompted to provide a title and detailed description (which becomes the commit message)
  3. review process
    • Reviewers examine the consolidated changes in the submission
    • Feedback is provided on the squashed commit
  4. iteration
    • Developer makes additional changes to their feature branch based on feedback
    • When changes are complete, they "resubmit" the PR, updating the commit message if necessary
    • System creates a new squashed commit with updated changes, replacing the existing one.
  5. finalization
    • Once approved, the squashed commit can be merged to the main branch

This approach maintains a clean history in the main branch while still allowing developers flexibility in their working branches.

meta notes#

These changes heavily depend on knots supporting the new /:did/:repo/merge and /:did/:repo/merge/check endpoints. To identify whether a knot supports pull requests—and subsequent features like search, code navigation, … etc. in the future, we're planning to introduce a capability model.

This could look something like knot.example.com/meta/capabiities which returns a map of features and their versions. For instance:

{
  "pull_requests": "v2",
  "code_search": "v1",
  "foo_bar": "v3",
}

This then allows the appview to conditionally enable operations and/or disable certain functionality on repositories hosted on knots that don't support them.

v2.2.2: There whould be three options (like on GitHub):

  • "Create a merge commit": All commits from the PR will be added to the base branch via a a merge commit.
  • "Squash and merge": The [N] commits from this branch will be combined into one commit in the base branch.
  • "Rebase and merge": The [N] commits from this branch will be rebased and added to the base branch.

The thing is, we want to eliminate the whole cognitive overhead (for the reviewer) of having to worry about the branch containing a "clean" history or not. What is presented for reivew is always a single changeset. The PR submitter is free to use their branch however they see fit—that's never seen by the reviewer.

Hence, consequently, "merge commits" and "rebase and merge" are both eliminated from the picture entirely. There's always just a singular merge of the PR submission—it's only ever one commit. This also forces changes from blowing out of scope.

I think giving the option in a dropdown would still be nice - what ever the default may be.

dropping my thoughts on what i want the PR flow to look like. i want to introduce a turn-based review system, where every "submission" is immutable:

  • user first "forks" a repo, and makes some changes on a branch
  • user then "submits" to target repo when ready -- this could be as many commits as they like
  • this creates "round 1" of "PR 1"
  • the owner can then offer a review on "round 1"
  • the user then makes updates to their branch based on feedback

at this point, unlike github, the PR is not updated automatically with new contents. the branch should not be used as a working area. rather, the user can make the changes they want, to address the review, and "resubmit" the branch for review

  • this creates "round 2", with the patchset produced from the same branch
  • "round 2" may add, delete or rework commits, by means of simply new changes, or performing a rebase on the branch
  • review process continues

the things this offers over the standard github model:

  • reviewers can request that commit history be clean, and even comment on specific commits in the history
  • reviewers can use "range diff" to tell the difference between submissions, existing flows cannot do this

finally, at the point of merge, there will be 3 options as suggested by @shi.gg.

I'm glad yinz are thinking about more than just branch-based PRs!!!

Optionally, this can be extended to more than 1 commit (pass in -2, -3, and so on) and paste the resulting patch series into the textbox. This is of course, a very simple model that only facilitates smaller changes that can typically fit within a few small commits. However, we see this as a driver for contributions that probably would not have happened in the traditional flow simply due to the overhead involved.

Honestly, I see this style of contribution as being even better than branch-based PRs; this is starting to approximate the stacked diff approach that is used by a lot of larger organizations, and I think is an area that's critically under-served in the open source world right now.

I have spent a lot of time talking and thinking about this, specifically, so happy to say more if that's helpful. Doing it "for real" would involve slightly more work than this copy/paste UI, but the fundamentals are the same.

And I do think that the copy/paste UI is going to be good for those sorts of drive-by submissions.

hi! @steveklabnik.com linked me to this thread :)

For context, I worked on Meta's source control team for ~6 years where we built out a state-of-the-art code review flow, inspired by mailing list workflows but brought forward into the 21st century with a modern web-based UI. I would love to see a workflow like that exist in a widely-used open tool.

The workflow involved:

  • focus on individual "diffs" as the unit of work rather than branches or PRs -- one diff = one commit that is proposed for inclusion
  • each diff is quite small, recommendation was around 300 lines or less -- again, similar to mailing list workflows where each patch is small, but scaled up a bit since diffs are reviewable on the web
  • amends and rebases locally, the idea being that you're writing your commit as you want it to appear in main. We built out extensive support in Sapling/Mercurial for doing this well, and Jujutsu does this even better while being Git-compatible)
  • first-class support for stacks of arbitrary depth -- it was common for devs to manage stacks of 10+, and it scaled well to 40-50+. (Think of how mailing list workflows often do PATCH [01/50] etc, except on the web.) Stacks were independent diffs, but there was a UI in each diff showing the full stack.
  • first-class support for merging stacks that have been accepted in code review via a "Ship Stacked (N)" button in the web UI -- this also worked with partial stacks for a "pipelining" workflow where you merge earlier parts of the stack while working on later parts.
  • all of this was API-driven and accessible via a command-line tool as well.

For all the years I worked on the team, the source control and review tooling received very high approval ratings in surveys (85%+) -- highest among all developer tools, and generally loved even as the outside world standardized on Git/GitHub and new folks coming in had no experience with any other flows.

At Meta we had a strong focus on trunk-based development so we didn't support long-lived branches, instead relying on feature flags. Some projects may want to support longer-lived branches, though. I don't have a strong opinion here but I think treating it as something different from patch-based code review makes a lot of sense. Personally I expect I would use patch-based tooling almost of the time.

I'd like to throw in my support for @steveklabnik.com and @sunshowers.io. I would love to start implementing this style in my own projects.

Hey, just dropping by to say I love the idea of the copy-paste patch submission, that sounds great for small quick changes!

Re: the proposal for v2, I think @oppili.bsky.social has some good suggestions, and I do think it's important to allow for changes that can build off of each other (stacked / branch-of-branch model), to allow for work on things that take more than a single smallish commit. If those commits have to be merged one at a time that's probably okay, but it can be useful to group a bunch of related changes together and review them either one-by-one or holistically.

Regarding this part of the proposal:

  • When changes are complete, they "resubmit" the PR, updating the commit message if necessary
  • System creates a new squashed commit with updated changes, replacing the existing one.

Sounds fine, but I wanted to highlight an important feature that I've been missing with Github for a long time: the ability to compare the before and after of this kind of "resubmit" action. On GitHub you get a force-push and comparing the changes is difficult (see e.g. this discussion). In my opinion this is hands-down the worst aspect of a rebase-driven workflow (or any kind of amend + force-push), and I think it would huge to be able to compare a "resubmission" like this with its original submission (or, ideally any previous version). Gerrit has a somewhat confusing interface and I don't love it, but this is one thing I think it does well — you can compare any two versions (I think they call it a patchset) of a given PR with each other, which is useful to see if e.g. your review comments were addressed, or some new bug slipped in due to a refactor after the PR was opened.

Anyway awesome work that y'all are doing and I appreciate that you're soliciting feedback from the community! Looking forward to v1 of this feature, I think it sounds like a great start!

I really like the copy-paste patch idea: it doesn't need branches and the amount of friction is acceptable IMO. Regarding v2, I'm fully with @ian-h-chamberlain.com. I'm working with jj and GitHub has a terrible interface for this rebase-style workflow.

I'm excited about the patch workflow, though on outside view I predict that as Tangled (hopefully!) grows it will go underused. IME less-engaged users always run into difficulty with uncommon git commands, even in very simple workflows like this one. But maybe this is the right mixture! 🤞

Folks above have discussed some dis/advantages of the mandatory-squash PR workflow; my 2¢ are that:

  1. It seems unnatural to me for a forge like Tangled to have an exclusive opinionated workflow, since part of the draw IMO is it can be deployed in many scenarios.
  2. While many (most?) contributors won't bother with a clean commit history in their PRs, I think keeping one is super helpful for communicating with the reviewer for all but the simplest of changesets, and I'd be sad to see that capability deliberately removed.

Reviewing differences-of-differences (like others have mentioned) would be great to have, though I wonder how you could implement them while both keeping most of the relevant data on the knot and not layering too much additional structure on top of vanilla git.

Anyway, thanks for opening this conversation up to the community.

I'm broadly excited by both the v1 and v2 proposals! For both, it would be extremely nice to have CLI-oriented interfaces to them, layered over existing git tools.

For example, for v1, you could create a git-send-email [1] variant (git-send-tangled) which accepts a tangled endpoint as an argument. git-send-email takes care of running git-format-patch, but the really painful bit of the workflow is that most users don't have a suitable email MUA to transmit it. This problem would disappear if the patch submission was via HTTPS to the Tangled appview instead!

For v2, again a CLI workflow would be really nice to have. Aside from git pushing and rebasing, the proposed idea of "PR submission" could be equivalent to a Linux "please pull from my branch" email that maintainers currently use. This would be a CLI that gives a base branch and commit, and a target endpoint, and it would just signal a request to pull to the upstream repo maintainer. The discussion/iteration over changesets would then be a separate CLI workflow, and has many more variations as is discussed elsewhere in the thread. By separating the "PR submission" from the discussion, it allows for easier experimentation on the latter, while following git-cli norms for the former.

[1] https://git-scm.com/docs/git-send-email

I'm very excited about the prospect of something like PR v1! Having a simple and easy way to propose small contributions is great and would get rid of the kind of scenario where you have a minor typo or the like to fix and it get lumped in with another change and/or never fixed because it's too small to bother with a whole PR for.

I want to throw my weight behind having the option to choose how the merge is handled like @shi.gg described. Forcing people into a specific work flow is part of what I dislike about a lot of forges. On top of that what different people, projects and organisations consider a "clean" history can very widely. Personally one big squash for the whole PR doesn't seem clean to me unless the PR is very small. To the degree where I'd prefer something like PR v1 instead. Supporting both merge commits and squashed commits also allows projects to have the best of both worlds. Using v1 for small changes, v2 with squashed commits for small-medium changes and v2 with merge commits for bigger feature additions and changes. In general I think the best system for this kind of things builds on top of the base the git itself provides without forcing it's own rules on top. Integrating with git rather than forcing it to do the systems bidding. For that reason i also really like @anil.recoil.org 's idea of integrating with git on the CLI.

I think the "turn-based" workflow that @oppili.bsky.social is describing is great! In my experience most projects effectively use that workflow but have to kind of work around the tooling due to how the changeset moves as the suggested changes pile on. Like @ian-h-chamberlain.com mentioned the force-push workflow on GitHub makes it hard to follow the evolution of a PR and it's changeset. Making "immutable changesets" that stack on top of each other would make the iterative process clearer to understand and easier to follow.

As a general thing what I would like to see is a system that caters to the way projects want to work instead of forcing them to work like the forge thinks they should. I'd rather have more options than have to cater my workflow to the tooling I use.

Thanks for all the comments folks. We've absolutely loved the ideas + suggestions in this thread. Super glad to see the copy/paste UI get such great reception; that's exactly what we intend to roll out shortly. We've made some tiny changes to the proposed v1 flow:

  • paste diff, but extracted from git diff instead of git format-patch—purely for user convenience. We plan to extend this to git format-patch in a future update which will auto-populate the commit title/body etc. (naturally).
  • round based reviews are here too! For each round, the user pastes a new, updated diff.
  • this will result in just a single commit in main.

As for v2, the key takeaways that I see from this thread:

  • people prefer having a choice of merge strategies—noted! We'll have a drop down.
  • the PR itself won't squash down to one diff—but this is still "up in the air" in terms of how we'll actually proceed. We'll share more on this as we progress.
  • changesets + range diff (to see what's changed between changes). This might even land sooner, before v2.

And as for CLIs, I agree that they're handy. One way to go about it would be to create a simple curl script that puts an sh.tangled.repo.pull record on the PDS. We of course, have to add support on our side to watch the firehose for these events and update PRs accordingly—but we're definitely doing that next… so it's simply a matter of time.

Once again, thanks everyone for chiming in! We definitely want to repeat this "public proposals" process for future platform changes. Meanwhile, keep a look out for our PRs v1 announcement very soon. :)

[deleted by author]
sign up or login to add to the discussion
Labels

None yet.

assignee

None yet.

Participants 11
AT URI
at://did:plc:hwevmowznbiukdf6uk5dwrrq/sh.tangled.repo.issue/3lkeinu32es22