Pmbootstrap: Workflow: refrain from using github web ui to merge pull requests

[alive4ever]

Hi @ollieparanoid , @MartijnBraam and committers of postmarketOS.

It seems that the workflow to merge pull requests is using github web interface instead of fetching pull requests locally and performs merging process with locally pulled PRs using git command line tool.

I suggest to refrain from using github web interface to merge pull requests and do the merging of pull requests locally for the following reasons:

1. Using the web interface, github will modify the author committer email to username(at)noreply.blablabla, which is probably not desired by pull request submitter. Github also modifies timezone information, which is acquired from github profile instead of timezone used during commit. When merging locally, the author information (including email and timezone) are kept.

2. The main source git repository should be @ollieparanoid's local git repository instead of github repository. Github should only mirror what's in @ollieparanoid local git repository (not vice versa).

3. Locally merging PRs should give flexibility to use the power of git command line tool (interactive rebase, etc).

[PureTryOut]

1. I honestly don't think many people will care. I never heard anyone about this in any project ever, you are the first one. As long as it's clear who made the commit (which it is), this shouldn't matter much.
2. Completely disagree. This would mean only @ollieparanoid could merge pull requests, which isn't doable for a project like this (or any large project for that matter). And no, we're not going to send patches via email like the Linux kernel lol. This project is maintained as a group, not just by @ollieparanoid.
3. Not sure what benefit that is to us, but sure

[bhush9]

I honestly don't think many people will care. I never heard anyone about this in any project ever, you are the first one. As long as it's clear who made the commit (which it is), this shouldn't matter much.

Sure, but current github workflow is linking commits to github accounts and not persons by changing the commit metadata to noreply emails. So in the worst case scenario of..

1) Github disappearing
2) original author disappearing from Github
3) postmarketOS moving to different Git hosting platform

You will have no way of identifying the commit author.

[MartijnBraam]

Github only changes the metadata to noreply addresses if you have your e-mail adres hidden on your github profile. For example @bhush9 and @ollieparanoid and me don't have an hidden e-mail adres on github and thus have the correct metadata in the repositories.

[bhush9]

So e.g

https://github.com/postmarketOS/pmbootstrap/pull/1115/commits

I see the commits in all of them have a author..

https://github.com/postmarketOS/pmbootstrap/commit/2b176cc549c9f8ccbb911d6b2ef848e13a0f8dfc.patch

But.. the resulting commit https://github.com/postmarketOS/pmbootstrap/commit/71d21a20c866f7a36c1e309287d6014e7fc0cfa8.patch

(even when it was squash, it resetted whole of author information to github account.. Which I find very uncomfortable tbh.. Github is propertiory software service and in cases of what I mention in previous comment this can be just disaster tbh)

[PureTryOut]

Well if the issue was about switching away from Github because it's a proprietary platform, I'd be totally for it. I even suggested this like a year ago, can't remember what the response was back then though.

However, as @MartijnBraam already said, as long as you set your e-mail to public, Github won't reset the metadata. Sure, it's bad Github does this in the first place, but this in itself is in my opinion not a good reason to switch the workflow around to something (in my opinion) more involved.

[ollieparanoid]

It seems that the workflow to merge pull requests is using github web interface instead of fetching pull requests locally and performs merging process with locally pulled PRs using git command line tool.

True, that's how we do it now.

1. Using the web interface, github will modify the author committer email to username(at)noreply.blablabla, which is probably not desired by pull request submitter.

I think the reason for GitHub modifying the metadata at all is, because we always use squash and merge.

The reasoning we currently have for doing it that way is stated in the wiki's FAQ:

Why do we use squash 'n' merge for pull requests?

First of all, we'll keep your comments in the commits, as long as they make sense in the squashed commit (no "fixed checksums" etc).

The big reason for "squash and merge" is, that the reviewer usually tests and reviews the last commit in the branch only. After all, that is the commit, that the users will run. Testing everything in-between would take a lot more time, which could be spent better elsewhere in the project. Besides that, it has the following advantages:

• Consistency with all other PRs
• Code is fully git-bisectable
• Less noise in the git log (also useful for git blame)

I am not a git expert to be honest, and while I'm writing and researching, I realize that the "code is fully git-bisectable" argument is misleading. It sounds like it would not be possible to properly bisect a branch with a regular git merge which is incorrect. So I'll update that part of the wiki when we find consensus in this issue.

But the "less noise" argument stands. It's much easier to read through the history with git log (compare the bottom with recent commits) and with the GitHub UI (before: look at PR #23 scattered over 12 log entries, after). (Not saying that the GitHub UI is more important.)

As I understand, @alive4ever and @bhush9 would prefer if we did not squash the commits, so all commits end up in the repository like they were made by the authors. So I would like to know, is there any way to keep it that readable, but without changing the metadata?

I know that we could ask the users to squash the commit. But then again seeing which changes were made in a pull request after a review is crucial for me. Let's say there's an update for many packages, and I'm looking through all the packages for half an hour, and find 5 lines I recommend to change. Then I would like to look at the diff since I did that review instead of looking at everything again. So... that's not really an option imho.

1. The main source git repository should be @ollieparanoid's local git repository instead of github repository. Github should only mirror what's in @ollieparanoid local git repository (not vice versa).

I use it both ways actually, when pkgrels need to be increased after a soname bump for example, then I do it locally with git, commit directly to the master branch, and push it. Actual code changes go through the review process as PRs.

1. Locally merging PRs should give flexibility to use the power of git command line tool (interactive rebase, etc).

I use interactive rebases a lot. When there's a merge conflict in a pull request, or the testsuite is failing when not basing on master (which is often the case), I resolve the conflict locally, then I push with --force to the author's branch and then I merge it.

---

@PureTryOut: Moving away from GitHub was discussed in https://github.com/postmarketOS/postmarketos.org/issues/37.

@bhush9: I share your view of the danger of GitHub being a proprietary service, and I think to be really in control of our own data, we would need to host our own infrastructure. But the most important part of what's here on GitHub is the code (with the commits that actually made it in, not some intermediate version that was good enough for merging) in my opinion, and that is well distributed through git. In the issue linked above we came to the conclusion, that self-hosting isn't worth it at this point (yet).

For the wiki we went with self hosting + backup on GitHub. Maybe an easy backup strategy in case GitHub turns against us, would be a cronjob that backups all the metadata GitHub has to gitlab once a day/week?

---

@alive4ever: Long story short, I've mostly tried to explain the workflow we have now and for which reasons. I'm open to improving the workflow as always, and I'm glad that you've opened up this issue, so we can all talk about it!

PS: If the issue is really just about the e-mail, maybe the simplest fix would be writing a new pull request template (#1319), that states something like:

If you want to have your e-mail address in the commit metadata, please make sure that you don't have the option checked to hide your e-mail address.

[alive4ever]

Completely disagree. This would mean only @ollieparanoid could merge pull requests, which isn't doable for a project like this (or any large project for that matter).

Make sense. Committers should always pull latest changes from the repository and rebase their changes before pushing to the repository so the repository doesn't get messed up.

What I suggest here, committers should merge pull requests locally using git command line tool, such as git fetch origin pull/1/head:pr1, git checkout pr1, git rebase -i, git checkout master, and git merge pr1.

As I understand, @alive4ever and @bhush9 would prefer if we did not squash the commits, so all commits end up in the repository like they were made by the authors. So I would like to know, is there any way to keep it that readable, but without changing the metadata?

I don't mind squashed commits before merging. What I don't approve is squash+merge via web ui.
Therefore, I suggest performing squash+merge using git command line tool.

Squashing commits can be performed using git rebase -i after checking out into locally fetched pull request instead of using squash merge feature on web ui. Using git command line will preserve the information as is.

After locally merging pull request, the related pull request can be closed manually. A simple git push will update the repository with locally merged pull requests.

[ollieparanoid]

Thanks for providing the detailed instructions. I will test doing it that way the next days (not in the pmbootstrap repo), and report back if that would work for me or not (I'm still not sure how the history will look like etc.).

@alive4ever: I guess you prefer if we wait with merging your PR until then, or is it OK for you if we use the current workflow for now?

[ollieparanoid]

Alright, after giving this some thought, here's my suggestion to solve this:

We can do both solutions. If we add a new pull request template, where the user can fill out whether they like to get it merged from the command line or with the GitHub merge button. Something like:

[x] Merge on GitHub (see <https://postmarketos.org/merge>)

The URL would point to a wiki page, that explains the advantages and disadvantages of each method, and that it is possible to allow displaying the e-mail address in the GitHub profile.

When the user changes it to [ ], then we would use the command line. Otherwise we use the faster GitHub click solution for people who don't care.

Is that fine with everybody?