Packages: [RFC] Github: using 'Rebase & merge' as a default merge strategy

[commodo]

Hey,

Just raising some thoughts.
If there is no agreement here, then also fine.
I'll close this in 1-2 weeks if there is no conclusion on this.

So, Github allows 3 merge strategies.
Any thoughts on changing default to Rebase & merge?
Reason is: git history looks like this [1] Too many merge commits with single commits.

If it is a good idea, then it's in Settings:

Git history of packages [1]

[champtar]

In Gitlab you can rebase, wait for CI again, then merge, which I really prefer.
Rebase remove the history so you don't know what was really tested, so I'm a bit against it, and also it removes the information of who took the decision to merge in git.
I'm totally against squashing because the submitter should take care of sending proper commit, it's not the job of the guy that merge to cleanup.
=> ok to have both merge or rebase, I prefer merge by default

[commodo]

Maybe as a quick addition: “Rebase & merge “ is like going back to the old way (or how OpenWrt core does it) where a maintainer applies a patch (which was sent via mailing list)

[champtar]

But the maintainer was doing rebase, test, merge, now we will do test then rebase & merge

[aparcar]

I asked the same question over at LuCI, just for reference.

I'm preferring the rebase strategy and already apply that whenever I merge anything.

GitHub stores both the information of who did it (the committer)...

... and also which PR it was:

I'm in favour to disable anything but rebasing and have "cleaner" commit log.

[aparcar]

@openwrt/package-maintainers ping

[hnyman]

I have lately been using "rebase & merge" in LuCI when there a single commit in the PR, but have used merge for the more complex stuff.

I would prefer to keep both "rebase & merge" and "create merge commit" as options.

Default could be "rebase & merge" if it is possible to select a deafult.
Note that Github remembers your personal selection, so it will offer the next time the same style for you in any case.

(And they have now introduced also a third option, "squash and merge" that I dislike.)

[rmilecki]

YES, PLEASE

Rebase remove the history so you don't know what was really tested

It doesn't squash commits, so you still see each of them (assuming there is more than 1 at all). It may result in loosing some context only when dealing with conflicts. That's pretty rare for packages I believe.

[aparcar]

@neheb As you do quite a bi chunk of the merging here, please comment

[neheb]

I'm OK with it. Rebase and merge still keeps context (PR number). I've merged PRs that way before but haven't really thought about git history and whatnot.

[aparcar]

I'd suggest to disable merging too as even a rebase keeps the history. If there are objections I'd only disable squashing.

[champtar]

We had the 3 options enabled, I just disabled squashing for now

[neheb]

My latest merges have all been rebases. That should be fine I guess...

[aparcar]

@thess @feckert please join the fun

[champtar]

Rebase merge, we do have the person in git

commit 2e297d29c026cc2d7a53ded5d30f1a8c5429c9d2
Author:     Phil Eichinger <[email protected]>
AuthorDate: Wed Oct 28 15:11:28 2020 +0100
Commit:     Rosen Penev <[email protected]>
CommitDate: Wed Oct 28 17:49:48 2020 -0700

    at: bump to version 3.2.1

    Change upstream to official repository at
    https://salsa.debian.org/debian/at

    Signed-off-by: Phil Eichinger <[email protected]>

And the PR number only in Github UI

Normal merge

commit 6a7ea2e42a3de584b308cf7cf94a251f12b6a551
Merge: 805e00a78 e8dd5038a
Author:     Ted Hess <[email protected]>
AuthorDate: Wed Oct 28 09:42:32 2020 -0400
Commit:     GitHub <[email protected]>
CommitDate: Wed Oct 28 09:42:32 2020 -0400

    Merge pull request #13795 from philenotfound/lame_cpe_id

    sound/lame: add PKG_CPE_ID

commit e8dd5038a9226636cab1a0efd3f51c987b04ea64
Author:     Phil Eichinger <[email protected]>
AuthorDate: Wed Oct 28 11:56:50 2020 +0100
Commit:     Phil Eichinger <[email protected]>
CommitDate: Wed Oct 28 11:56:50 2020 +0100

    sound/lame: add PKG_CPE_ID

    Signed-off-by: Phil Eichinger <[email protected]>

[BKPepe]

If we would be using rebase & merge button, then the Verified check (verified GPG signature for commit) will be gone.

[aparcar]

@champtar How about --format=fuller?

user@dawn:~/src/openwrt/packages$ git show 2e297d --format=fuller
commit 2e297d29c026cc2d7a53ded5d30f1a8c5429c9d2
Author:     Phil Eichinger <[email protected]>
AuthorDate: Wed Oct 28 15:11:28 2020 +0100
Commit:     Rosen Penev <[email protected]>
CommitDate: Wed Oct 28 17:49:48 2020 -0700

    at: bump to version 3.2.1

    Change upstream to official repository at
    https://salsa.debian.org/debian/at

    Signed-off-by: Phil Eichinger <[email protected]>

If we would be using rebase & merge button, then the Verified check (verified GPG signature for commit) will be gone.

I don't see anyone in packages.git using this so is that really relevant?

[champtar]

@aparcar I'm using --format=fuller ...
Do we really want to forbid merge ?

[neheb]

@BKPepe GPG is a joke. The creator of GPG doesn't even use it. I don't see it as being relevant in git.

edit: I misspoke. PGP.

@champtar @commodo was complaining about the history created by merging, no?

[aparcar]

Do we really want to forbid merge ?

No I want to encourage rebases

@aparcar I'm using --format=fuller ...

Right, sorry I missed that.

And the PR number only in Github UI

Is that really relevant? Maybe it is, I'm just thinking about openwrt.git where that's not stored at all, patches come from GitHub or via mail.

[champtar]

openwrt.git workflow is rebase, hopefully test by the committer, then merge, but yes we are fine without any link to the source of the patch.

No I want to encourage rebases

I think we are all set then ;) https://github.com/openwrt/packages/issues/13654#issuecomment-712821237

[BKPepe]

I don't see anyone in packages.git using this so is that really relevant?

I would like to apologize, but I am not looking at git.openwrt.org, I am talking here about Github for packages repository, which is used for Pull Request. I see that there are several people without commit access and also people with commit access, which they are using GPG or S/MIME for their commits. I don't want to mention anyone, but here you have a few examples:

https://github.com/openwrt/packages/commit/5c53fe2ad3978d6b2b586aed3f5f2b80a9a95f22
https://github.com/openwrt/packages/commit/ba8748f9570cb7ec1c02b85bcf3603ccc7100f62
https://github.com/openwrt/packages/commit/d276c81ea81dac8d954f7606b44bdfb68548355b
https://github.com/openwrt/packages/commit/4b0d029bb535229c1d5f71a8d90b8f7994ca2ae0

Also @neheb, me are using that as well.

I would say there's a difference if you see this
or not.

[neheb]

It's interesting.

I have a GPG key uploaded to GitHub. Merges from me show Verified but merge + rebases from me do not.

Anyway, I don't value GPG so it's a moot point.

[Cynerd]

Anyway, I don't value GPG so it's a moot point.

And how do you plan to verify commits? I think that in distribution maintainer should always be reliably verified as it is the last guard before it is pushed to all users. How can you trust security of OpenWrt when source is not verified?

I somewhere even have code to do quite opposite in OpenWrt, to verify feeds by git (that is by pgp). Right now the only insurance we have is https. We trust you but should we also trust that servers hosting repository are impenetrable? Hardly. This is step backward from security, really.

I have a GPG key uploaded to GitHub. Merges from me show Verified but merge + rebases from me do not.

I am sure that you do not have private part on github for sure can't sign your rebase commits. Since content of commit changes in rebase it has to be signed again. Solutions are either merge commits or doing rebase locally (allowing only fast-forward merges on github without rebase allowed). Github allows you to modify branch user submitted for merge so there is no issue in rebasing manually instead of on github automatically. In Turris we have policy that tip has to be always signed by someone from Turris team.

And honestly it doesn't matter if gpg is terrible project, up to my knowledge it is the only way to sign git commits/tags, at least it is the way. So any gpg or pgp critique here is just baseless. Go and improve it or submit replacement to git (it is open-source nonetheless) but right now it is about signing or not signing and I am saying: Are you out of your mind to make not-signing the policy?

[neheb]

Anyway, I don't value GPG so it's a moot point.

And how do you plan to verify commits? I think that in distribution maintainer should always be reliably verified as it is the last guard before it is pushed to all users. How can you trust security of OpenWrt when source is not verified?

AFAIK, nobody can rewrite history here. git --force push will fail.

I somewhere even have code to do quite opposite in OpenWrt, to verify feeds by git (that is by pgp). Right now the only insurance we have is https. We trust you but should we also trust that servers hosting repository are impenetrable? Hardly. This is step backward from security, really.

Maybe makes sense with GitLab. No idea.

I have a GPG key uploaded to GitHub. Merges from me show Verified but merge + rebases from me do not.

I am sure that you do not have private part on github for sure can't sign your rebase commits. Since content of commit changes in rebase it has to be signed again. Solutions are either merge commits or doing rebase locally (allowing only fast-forward merges on github without rebase allowed). Github allows you to modify branch user submitted for merge so there is no issue in rebasing manually instead of on github automatically. In Turris we have policy that tip has to be always signed by someone from Turris team.

And honestly it doesn't matter if gpg is terrible project, up to my knowledge it is the only way to sign git commits/tags, at least it is the way. So any gpg or pgp critique here is just baseless. Go and improve it or submit replacement to git (it is open-source nonetheless) but right now it is about signing or not signing and I am saying: Are you out of your mind to make not-signing the policy?

I don't see any value in signing commits. I only see value if rewriting history is a possibility.

[aparcar]

These Verified badges give a sense of security as it's a GitHub key. It just verifies your merge on the fly, this has nothing to do with uploaded GPG. Uploading any (public) GPG keys just seem to be a perk for the UI?

Currently more than 30 people have commit access on packages.git. If you want more security it should involve everyone. Just having a few people signing a commit every now and then doesn't really help.

Anyone modification of the git history would be detected and reported by multiple maintainers/developers. I don't see that as a big risk, correct me if this assumption is wrong.

It seems more likely that some account with commit access is compromised and pushes malicious code. We could require 2FA for all maintainers.

So, any convenience allowing modification of packages.git via the web interface means potential security issues. If you don't trust GitHub we can move things to git.openwrt.org and follow the same workflow as openwrt.git (which barely does any signing neither).

[BKPepe]

These Verified badges give a sense of security as it's a GitHub key.

What are you saying just applies to merge commit. You need to take a look at the commits, which I posted.
For example:

Those are not definitely GitHub keys.

[BKPepe]

Uploading any (public) GPG keys just seem to be a perk for the UI?

While using git in CLI, you can see if the commits were signed by GPG or not.
By using git show <commit> --show-signature and same applies to git show --show-signature HEAD^ or there is command git verify-commit <commit>

We could require 2FA for all maintainers.
Go for it.

We can move things to git.openwrt.org and follow the same workflow as openwrt.git (which barely does any signing neither).

You don't mean this seriously, right? Do you have any statistics for this? I would like to say that many people are using pull requests via GitHub and not many are sending patches via git send-email. Also, people want to have them as merged pull requests instead of showing them as closed.

[aparcar]

Okay I'll leave the decision to the rest of the (maintainer) group. I think it's good to disable squashing of commits in PRs and agree that it's bad style to rebase a signed commit. However as a high number of commits is unsigned, rebases should be preferred for these cases. If there was a signature added, a merge should be done to keep the signature valid.

Additionally, whoever wants to make it as their mission, could encourage people to sign their comments?

[dwmw2]

FWIW I detest rebasing as it lies about history.
If I test my changes and commit them, then that is what should go into the history. If something subsequently breaks, I should be able to bisect from that known working point.

Rebasing breaks that by throwing away the true history which was really the whole point of a version control system. Instead of having that fixed point preserved accurately, we get to take our chances with whether it ever works or not.

And yes, those failure modes aren't ever so common but they do happen. Maybe someone updates a library or function that I'm using in the meantime and my commit no longer works. And by the time we come to work it out, it looks like my commit never worked because we didn't use git in the way that it is actually supposed to be used; we threw away the true history.

Every time this happens to me I am livid with whatever project I'm working on, which uses git incorrectly. Use the system properly; never rebase.

Please, let's not be one of those projects that screws it up and doesn't even understand the basic concept of what version control is actually for.

[dwmw2]

Reason is: git history looks like this [1] Too many merge commits with single commits.

In case I was too ambiguous about how much I don't agree with this proposal, let me say also that I object to the very premise of this observation. The history that you see there, "messy" as it is, is the truth. It is the true history, and preserving that is what we have a version control system for. If we want to ditch that because the true history is hard to handle, why bother with version control at all? Why not just throw tarballs over the wall?