On Sat, Nov 30, 2019 at 7:37 AM Angel Pons th3fanbus@gmail.com wrote:
Hi everyone,
On Sat, Nov 30, 2019, 14:15 Nico Huber nico.h@gmx.de wrote:
Hi all,
the GitHub PR topic popped up from time to time on IRC in the past but it seems we never discussed it here or came to any conclusion that led to action.
When we switched to Git, we wrote down three ways to send patches [1]:
o Via our mailing list
o Via gerrit on coreboot.org
o Via pull request on flashrom's github mirror
If the repo on github is a mirror which tracks the "master repo" in coreboot, it does not make sense to use pull requests, as they would land on the mirror.
Correct - The flow is to move the PR to Gerrit, get it merged, and then it shows up in the mirror. One nice thing about Github is that it is smart enough to automatically close PRs that get merged this way.
I have never reviewed on the mailing list, so I don't know how tedious it is. In any case, it's easier to move mailing list patches to gerrit than pull requests.
Now, roughly 2 years later, some PRs have been merged, but some, even smaller ones, were left unanswered. We also never set a clear process how to move things to Gerrit.
For what it's worth, here are the steps I use to manually move things to Gerrit: # Clone flashrom from upstream and add the github mirror as a remote: git clone ssh://<username>@review.coreboot.org:29418/flashrom.git cd flashrom git remote add -f github git@github.com:flashrom/flashrom.git
# Create a new local branch with the PR applied git fetch github pull/<PR#>/head:<new_branch> git checkout <new_branch>
# Push it to gerrit with a topic indicating what PR it's from git push origin HEAD:refs/for/master%topic=<PR#>
Agreed. Should we stop accepting pull requests, it would be good to document (or link to) the procedure to create a change on gerrit.
I think it's useful to have a Github mirror since that's where a lot of developers are. However, we should make presubmit hooks more strict to increase likelihood of a patch getting merged with minimal feedback needed from reviewers. This is true with Gerrit as well, of course, but especially Github since a lot of one-time contributors send patches that way.
Another way which Github is useful is for reporting issues, asking for a release, etc. AFAIK the closest thing we have for flashrom is the mailing list, but of course that's far less convenient for most people who have an account on Github and can easily open an issue that way.
While I still don't object to reviewing on Github, if somebody wants to do so, it has some downsides: no global overview of pending patches, no build testing before moving things to Gerrit, the overhead of moving things, ofc (IMHO, reviewing on Github is also much harder, maybe I'm just Gerrit spoiled).
IMHO, handling the merging of pull requests alone is cumbersome enough to outweigh the benefits of allowing pull requests.
I prefer the Gerrit UI for the reasons Nico cited and also the subjective feel of it. That said, I don't think that handling PRs and merging them is a big deal, so long as they're simple and don't need a lot of back-and-forth.
Especially the build-test integration makes it hard for me to come up with a reasonable process. Hence, I suggest that we just stop accepting PRs on GitHub and tell contributors to push to Gerrit directly. This may be more work for the contributors and might even scare some away; but I don't see any lack of contributions to this project rather a lack of reviewer resources. So we should make reviewing as easy as possible, IMO.
I completely agree.
Seems a bit extreme IMO, but I'm not totally opposed to it if PRs are deemed a big enough nuisance. Perhaps we can start by allowing PRs, but say that anything complex will need the author to submit directly to Gerrit? I think people who want to make multiple, complex changes will be OK with that and it keeps things easy for one-time contributors who just want to add a flash chip or fix a small compilation issue or something. If we don't get the desired result then we can try disabling PRs entirely.
In any case, we should start by documenting the process to send a patch to review.coreboot.org using Github credentials. I've added a section for this at https://www.flashrom.org/Development_Guidelines. For now I just wrote what I described here, but if we disable PRs entirely we can update that section to reflect that.
We should also figure out a way to make info relevant to Github users more obvious. Currently Github displays our generic README by default, I'm not sure if there is a way to change that without removing the README from the repository. I also updated the heading on Github to point straight toward the development guidelines.