Pull requests
(1.1) By Warren Young (wyoung) on 2018-08-16 08:27:50 edited from 1.0 [source]
This thread picks up from another, which drifted well off its original topic.
This feature request has come up many times before: [0] [1] [2] [3]
I want to concentrate all of that prior thought and crystalize it. I think we'll soon be in a very good position to finally move on this. If it turns out that we can't get this soon, this forum thread will at least provide a central resource to point people at when they inevitably ask for such a feature yet again.
The Wish
We want something like GitHub pull requests in Fossil, but we don't want to turn Fossil into Git or GitHub to get it.
Terminology
The term "pull request" implies either:
A centralized service like GitHub, where all of the involved repositories are held within that single service, so that one repo can pull directly from another. I don't think we can expect Fossil to evolve in this direction: if it did, it would fork Fossil into two very different VCSes. Fossil is a true DVCS, and many of us like it that way.
Both the sending and receiving Fossil repos are public, and at least one user of each repo has access to the other. That results in a federated model of development like that of the Linux kernel. I don't think we want to require such a setup just to get some variation on GitHub pull requests. (This is how
git pull-request
works, by the way.)
We don't have to accept the problems with either of these. We just have to flip it around, and it becomes easier to solve while ending up scarcely more difficult to use than GitHub pull requests.
I want to call our alternative a "push request," because it emphasizes the fact that there is an important difference in outlook between Fossil and Git/GitHub.
Others have called this a merge request instead.
Unprivileged Workflow
An outside contributor does their work on a branch in a clone of the central repository, which was made as a Fossil user without checkin rights. Some of the methods below even allow anonymous clones. The outside contributor should disable autosync on that clone's checkout so that all checkins go into the local clone repo only without taking time to repeatedly fail on sync.
When the outside contributor is ready to push their change up to the central repo, they say something like:
$ fossil bundle push --branch my-new-feature
And Then What Happens?
We have many possible ways to go from there, all well within our reach:
Append Unversioned — This is a new capability (I propose
U
) which the central repo's admin assigns to users on request. It allows remote Fossils to upload a bundle to the central repository without also granting them the ability to delete or replace existing unversioned files, as the current Write Unversioned capability (u
) allows.(I do not recommend giving this capability to
nobody
, since that would allow spam in quantities sufficient to DoS the central server. I also do not recommend giving it toanonymous
since the CAPTCHA mechanism only works via Fossil UI, and we don't want to require push requests via the central repo's web UI only. Separately, the central repo's admin will likely wish to have the option to revoke this capability from an individual user who abuses their privilege, so the privilege must be granted on a per-user basis anyway.)The outside contributor's Fossil instance uploads the bundle using a special naming convention (e.g.
pr-bundle-abcd1234efg...
) that causes the central repo to notify interested parties of a new push request. Someone with checkin rights on the central repo could then say:$ fossil bundle import --uv abcd1234
That syntax would make
fossil bundle
look for the bundle in the unversioned file table, not the local filesystem. The argument is a unique prefix of the hash part of the unversioned artifact's name. It could also be a prefix of the blob ID or the file's SHA-1 hash.If the committer likes the push request, she can then say:
$ fossil bundle import --uv abcd1234 --publish
to merge it into the central repo.
Push to Forum — On
bundle push
, the outside contributor's Fossil instance opens a text editor for a forum post, just as for composing a checkin comment. That then pushes the bundle to the remote Fossil instance, making use of the existing attachment mechanism to hold the bundle. The central repo's admin then needs to grant only Write Forum permission to the user doing the push request.(Implicitly, this means the current forum feature needs to allow file attachments. Again, the underlying mechanism already exists, someone just needs to enhance the Fossil forum feature to make use of it.)
The committer then says:
$ fossil bundle import --forum abcd1234
and all proceeds as before.
By doing this through the forum, others can comment on the push request before it's merged. (And after, of course.)
This method gets you most of the way toward another oft-requested feature: code reviews. It might be close enough as presented here for some users.
The easy way to work this is for the central repo to be the same as the forum repo, but in cases like the current
fossil-scm.org
one, we can add a CLI option to cope:$ fossil bundle import --forum abcd1234 -L ~/museum/fossil-forum.fossil
or
$ fossil bundle import --forum abcd1234 -L ../forum
The
-L
flag gives the path to the Local forum repo clone or to one of its checkouts. It is named by analogy to-L
and-R
in OpenSSH, with the mnemonic changing from R=repo to R=remote repo.Push as Ticket — This method has some workflow and cosmetic advantages over doing it via the forum.
The command differs only trivially:
$ fossil bundle import --ticket abcd1234
That will pop up a ticket submission template in the submitter's text editor. It could have predefined fields for the key user-supplied ticket fields: summary line, severity, subsystem, etc.
For fields presented in Fossil UI as drop-downs, if the submission uses text that doesn't match one of the predefined fields, the receiving Fossil instance should probably just drop the text rather than insert the text as given, since that would screw up existing ticket reports. The ticket triager can fix it up, since they're likely to need to make other edits to the ticket anyway.
Push via Email — The outside contributor configures their local Fossil with their local email setup via global settings, allowing
bundle push
to send the bundle off as an attachment to an email message. The email goes to an address configured on the central clone, likefossil-bundles@example.com
, which is a bit of new metadata that comes down with the clone.The major advantage of this option is that it doesn't require that the central repo give out a user capabilities before it can accept a push request. The remote user also doesn't have to re-sync using a user account, which they might need to set up before they can commit, which adds friction to the process, increasing the likelihood that your new committer will give up before getting the contribution to you.
One disadvantage is that this method is easily spammable, unlike the prior options.
This might be a more popular use of the direct-to-SMTP email method than on central Fossil servers, since central Fossil servers often run on machines that already need a
sendmail
type setup of some kind, so that's the low-friction path.Push to Custom HTTP Endpoint — The above methods have the advantage of reusing existing mechanisms, but what if the central repository doesn't want to give out the capability bits or open the services required? Maybe they'd prefer to reserve those for other purposes, and would welcome a method to control this push request separately.
Fossil could offer a
/push
service, which would accept an HTTP POST from the outside contributor's Fossil instance, gated by a new user capability. Call it Push Request. I propose using capability letterP
for this.
"Bundle Push"?
There are other ideas for the exact Fossil command the submitter gives.
I think bundle push
is consistent with the existing bundle
feature set, but perhaps it focuses too much on the mechanism. It might also be nice to have a clear sub-command show up in fossil help
output, rather than hide it behind bundle
.
Another idea I haven't seen before is to extend fossil push
:
$ fossil push --branch my-new-feature
$ fossil push --branch my-new-feature -L ../forum
$ fossil push --branch my-new-feature -L ~/museum/fossil-forum.fossil
The first pushes to a forum-and-code repo, and the latter two are as above, for use with separate forum and code repos.
I like extending the push
verb because in an outside contribution model, autosync is likely to be disabled on outsiders' checkouts, so an explicit push is a more useful concept than when direct commits are allowed.
Multiple Methods?
We don't necessarily have to pick only one of the methods above. We might like to have several options available. The push
command would accept the same flags as in the bundle import
examples above:
$ fossil bundle push --branch my-new-feature --ticket
$ fossil bundle push --branch my-new-feature --email fred@example.com
$ fossil bundle push --branch my-new-feature --forum -L ../forum
The bundle import
method must match the bundle push
method, of course.
Fossil UI Integration
I've given all of the above in terms of the CLI, but some of the methods allow for Fossil UI integration: push via /uv
, forum, and ticket, in particular. Push via custom HTTP endpoint could also work with this, though it would require new UI work.
For the most part, I think the central repo's committer wants to work at the command line for this, since they're going to need to build and test the contribution before accepting it anyway.
However, there are some small advantages of doing the actual PR acceptance in the GUI, basically amounting to closing the feedback loop:
- Delete the
/uv
artifact. - Reply "Merged, thanks!" to a commit-by-forum PR
- Comment similarly on a commit-by-ticket PR and close the ticket.
It is possible that some pushes are so obvious that they could be inspected purely in Fossil UI and accepted right there. A common example would be a typo fix in the project docs or in a source code comment.
Private Branches
None of the above requires use of private branches. You can do it that way if you like, but it'll work with un-sync'd "public" branches, too. Using private branches instead is just a small workflow enhancement to avoid "not authorized to push" complaints from Fossil.
(2) By Florian Balmer (florian.balmer) on 2018-08-16 07:35:44 in reply to 1.0 [link] [source]
Please allow me to forward some more thoughts about pull requests to the new forum:
fossil-users: Using bundles for pull requests
Concerns:
- The bundle import logic should protect against random control artifacts, and delta artifacts referencing yet-to-be submitted control artifacts.
- Bundles derived from local private branches can introduce "unknown" artifacts, and may also lead to potential problems with future merges, because the parents and merge ancestors referenced by the imported check-ins may be missing.
Wishlist:
- Decide where the bundle is applied ("rebasing").
- Decide whether to apply the bundle step-by-step, or in one single commit ("compressing").
- Allow for minor "on-the-fly" fixes to comments and coding style prior to importing the bundle.
- Option to import only the contents, and add custom check-in comments, branch names and other tags, completely ignoring the meta-data from the bundle.
(3) By Roy Keene (rkeene) on 2018-08-16 21:23:31 in reply to 1.1 [link] [source]
I'm in favor of pull request via bundle artifacts. I would use this functionality occasionally.
(8) By Roy Keene (rkeene) on 2019-01-12 20:18:55 in reply to 3 [link] [source]
Here's basically what I think I'd like:
Database (high-level):
- Each ticket allows 1 unversioned bundle per user (excluding nobody)
- Users may only write to this unversioned-bundle slot if they have write update access to the ticket
- Bundle may be replaced (or evicted) as many times as needed
- Administrators may write to any user's unversioned-bundle slot
- Unversioned-bundles are not synced
- Size limits may apply
- Optionally they can be cleaned up after the ticket reaches a state, and/or time passes
- Unversioned-Bundle changes by non-administrators may be rejected for tickets in certain states (Fixed, Closed, etc)
CLI From External Contributor Usage (Writes to Ticket UV-Bundle Slot):
fossil pull-request push [-u <url-including-username>|<named remote>] [-b <branch>] [-s <start commit>] [<username-slot>/]<ticket-uuid?- maybe skipped if its already known>
<username-slot>
is to allow administrators to overwrite someone's unversioned-bundle slot
fossil pull-request rm [-u <url-including-username>|<named remote>] [<username-slot>/]<ticket-uuid>
CLI From Maintainer Usage (Reads from Ticket UV-Bundle Slot):
fossil pull-request pull [-u <url-including-username>|<named remote>] [-p <branch-prefix>] [<username-slot>/]<ticket-uuid>
<username-slot>/
may be omitted if there's only one slot in use<branch-prefix>
defaults to ticket 10 chars of the ticket UUID+username-normalized; mapping must be kept somewhere so that if a new pull request is created later from the same ticket; If there is only one branch in the bundle containing new artifacts, name that branch this prefix instead of prefixing it?
fossil pull-request cancel
to delete the artifacts created byrequest-pull pull
Web UI:
- Some way to inspect the contents of the bundle
(10) By Dan Barbarito (danbarbarito) on 2021-08-31 03:57:27 in reply to 8 [link] [source]
I really wish to see something like this in the future.
I recently tried getting a team to use Fossil for a small project but was met with a lot of resistance because there is no pull request functionality. Not the greatest reason to go with Git/GitHub but I was outnumbered.
(11) By sean (jungleboogie) on 2021-08-31 04:28:08 in reply to 10 [link] [source]
What would you friends think of the patch cmd? Here's a doc page on it: The "fossil patch" command.
It's not exactly pull requests but kind of in that direction. In fact, I could imagine someone writing a simple script to make a patch and upload it to a webserver and send a notification in the Fossil chat on upload with a link to the file.
(12) By Warren Young (wyoung) on 2021-08-31 12:57:50 in reply to 10 [link] [source]
Are you sure this wasn't just an exhibition of the narcissism of small differences? That is, was this cited issue a pretext, or would providing this lone missing feature have flipped their judgement?
I ask because your use of the word "team" clashes with my notion of what PRs are for: a way for outsiders to push proposed changes into a project they are on the outside of. Team members are inside, by definition, so why would you want PRs in the first place?
When I think of team-based development, I think of the very sort of organizational differences that push you toward direct commits and away from PRs. Why would you insert delays into your organization's OODA loops by requiring that internally-sourced changes be pushed up through a PR acceptance process?
Some tools build code review features atop Git's PR feature, but PRs are not the only way to do this. Is this what your antagonists are actually wanting? I still claim it's increasing the size of your OODA loop, but at least then we can have a sensible conversation about it. (On another thread, please.)
(13) By Scott Robison (sdr) on 2021-08-31 15:34:52 in reply to 12 [link] [source]
We use PRs at L3Harris in a closed proprietary team in our normal workflow. It's far more than drive by modifications.
(14) By Warren Young (wyoung) on 2021-08-31 15:55:41 in reply to 13 [link] [source]
Okay. How, exactly? Detail the process so those of us not employed by L3Harris can evaluate how Fossil might fill the same role.
(15) By Scott Robison (sdr) on 2021-08-31 16:14:57 in reply to 14 [link] [source]
Someone creates a branch. They work on it. They push it to the master repo and create a pull request. People review and comment. Changes may be made and pushed, lather rinse repeat. It is eventually rejected or accepted and merged.
It can be done in forked repositories as well but that is not a pre-requisite.
As for fossil supporting anything, I don't know that it needs to go by the name "pull request" because really what we're talking about is infrastructure for a formalized "code review & merge request" which may or may not need to be pulled.
My point was simply that the modern concept of "pull request" is not restricted to drive by contributions. Many teams have added that as a part of their workflow.
Whether I personally like it is another issue.
(16) By Warren Young (wyoung) on 2021-08-31 16:34:12 in reply to 15 [link] [source]
Right, so of what I'm seeing here, use of PRs here is a conflation of Git's available feature set and the high-level feature desire. You don't need "PRs" to achieve this, you need:
Some mechanism for code review.
Notification of a mergable branch so that code review can begin.
I think we can do both within Fossil today in terms of the forum feature. The way it'd work is that you give a command like:
cd ../trunk # from your working branch checkout
fossil merge --request my-working-branch
This would create a forum post named something like "Code review: branch my-working-branch", the body of which is a copy of the commit message and a link to its /info
page. The buttons at the bottom of each post would vary so that those with merge power are able to accept the merge request.
I think it's simplest if accepting a merge request via the forum merges tip-of-branch rather than the version current at the time of the thread's creation. This allows more work to happen on the branch until the ones giving feedback are satisfied. It's probably also best if the resulting merge implies --integrate
.
(17) By Scott Robison (sdr) on 2021-08-31 17:10:32 in reply to 16 [link] [source]
The one advantage of the PR methodology we use (it's not github) is the fact that comments can be tied directly to given lines in the review process.
But just to be clear: I'm not advocating for a PR mechanism in fossil, just observing that it is definitely used more widely through industry than just by people who fork open source projects.
Whether we use PR in an actual "git pull then git merge" sense or just as a "pull changes from one branch to another" sense, people want to use it.
(18) By Dan Barbarito (danbarbarito) on 2021-09-01 18:16:26 in reply to 15 [link] [source]
This was exactly what the team I'm working with brought up. They didn't want PRs for drive-by contributions but it was more about having something we can all reference for code reviews and discussions. I brought up that having a feature branch would provide the exact same thing (we can all just look at the diff of feature branch -> trunk) but that was not what the team was used to.
(4) By Florian Balmer (florian.balmer) on 2018-08-17 06:18:53 in reply to 1.1 [link] [source]
Another point worth considering: as user names can be freely changed and reused for local clones, it's possible to create bundles containing check-ins with arbitrary user names. The current bundle import logic does not verify that the user names exist, or that they really refer to the users with the same names in the upstream repository.
Currently, the process of accepting and reviewing submitted bundles seems tedious to me. Random control artifacts can have the powers of `fossil amend', and they can be nested, or scattered across multiple bundles.
(5) By Warren Young (wyoung) on 2018-08-17 13:53:39 in reply to 4 [link] [source]
user names can be freely changed and reused for local clones, it's possible to create bundles containing check-ins with arbitrary user names
Yes, that came up on the mailing list most of a year ago now. My stance then and now is that some central repos will want to have a setting that lets them either:
rewrite the identity claimed in the new artifacts to match the checkin user's identity on the central server; or
refuse the checkin because the claimed identity in all the new artifacts doesn't match the checkin user's identity on the central server.
I like these solutions because they're effective in the main use cases, and they're reasonably simple to implement.
What these solutions don't get you to is any kind of global identity solution. If that's what we want, then I recommend that someone reads that thread before replying here, because I brought up several options at the time. If we want the option of matching on global identity in Fossil, I think we should choose one of those existing solutions rather than try to make something up ourselves. Identity is complicated, and others have thought long and hard on this. This is one of those areas where we should delegate, rather than reinvent.
One big reason not to reinvent this wheel is that global identity without centralization tends to fail in practice. (e.g. PGP key distribution.) Fossil is in no position to drive a new global identity standard.
And that brings me back to my solutions above. I think implementing global identity matching in Fossil is probably more work than it's worth. Doing so would solve problems that are probably entirely theoretical at Fossil's current scales of adoption and use. I don't see why we can't defer a solution until Fossil does start getting used in scopes and schemes that require global identity matching.
I don't see this push request idea as requiring it. My simpler solutions above will suffice.
(6) By Florian Balmer (florian.balmer) on 2018-08-17 16:24:57 in reply to 5 [link] [source]
Yes, that came up on the mailing list most of a year ago now. My stance then and now is that some central repos will want to have a setting that lets them either:
- rewrite the identity claimed in the new artifacts to match the checkin user's identity on the central server; or
- refuse the checkin because the claimed identity in all the new artifacts doesn't match the checkin user's identity on the central server.
Thanks for linking to the interesting discussion.
My additions:
- Currently, when importing a bundle, the "rcvfrom" table has an entry for the user (uid) who imported the bundle, and not for the one who "created" or "submitted" it.
- The "rcvfrom" table only stores information about the current repository. With inter-repository cloning and synchronization, the "rcvfrom" table holds entries about the artifacts received by the cloning/syncing operations, but none about the artifacts received by the originating remote repository.
(By the way, with inter-repository synchronization as the recommended backup strategy to prevent issues of "traditional file copies" with hot database journals, the original "rcvfrom" information will also be lost, if a backup needs to be restored.)
(7) By Dan Barbarito (danbarbarito) on 2018-08-24 19:31:20 in reply to 1.1 [link] [source]
Any thoughts on this, drh?
(9) By anonymous on 2020-01-28 01:20:17 in reply to 1.1 [link] [source]
- Push as Ticket
I like using tickets as the vehicle.
Now that fossil has the /ext feature, I envision a "Pull Request by Ticket" extension to Fossil.
After a user has pushed their changes to their publicly facing Fossil repo (like, for example, chiselapp), they goto the timeline page on their public repo, click on the leaf of the branch they want to have pulled, then click on a new link, "Send Pull Request".
That link would be to a CGI in /ext: $REPO_BASE/ext/pr_create.cgi/commitID
pr_create.cgi would:
Make a bundle:
fossil bundle export $ID --branch $ID --standalone -R $FOSSIL_REPOSITORY >pr/$ID
Read a Pull Request ticket template.
Insert a URL:
$REPO_BASE/ext/pr/$ID
Optionally pre-fill some other info.
Send the partially pre-filled form to the user's web browser.
The user then finishes filling out the ticket form and clicks submit.
When the owner of the parent repo sees the ticket, they can click on the link to fetch the bundle, then proceed from there.
The above presupposes that Fossil can push tickets even when the user lacks commit permission to the parent repo (and when "dont-push" is set TRUE).
Failing that, the template would be an email template and there would be an ext/pr_send.cgi
that would, somehow, email the completed form to parent repo's owner.
I might experiment with this. If I do, I will post the results.