GitHub PRs

[Nico Huber]

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

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.

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).

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.

Nico

[1] https://www.flashrom.org/Development_Guidelines#Sending_a_patch

[Angel Pons]

Hi everyone,

On Sat, Nov 30, 2019, 14:15 Nico Huber  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.

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.

Agreed. Should we stop accepting pull requests, it would be good to
document (or link to) the procedure to create a change on gerrit.

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.

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.

Nico
> [1] https://www.flashrom.org/Development_Guidelines#Sending_a_patch
> _______________________________________________
> flashrom mailing list -- [e-mail]
> To unsubscribe send an email to [e-mail]

Best regards,

Angel Pons

[David Hendricks]

On Sat, Nov 30, 2019 at 7:37 AM Angel Pons  wrote:
> Hi everyone,
>
> On Sat, Nov 30, 2019, 14:15 Nico Huber  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 [e-mail]: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.