[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
[rdf4j-dev] merge strategy: back to merge commits?
|
I'm very sorry to have to open the "merge strategy" debate yet again, but: I think we may need to consider going back to using merge-commits as our default (and only) strategy for merging pull requests.
Although I personally like squash-and-merge, as it is an easy way to give us a clean, linear history, there is a major problem with how Github implements the feature: it changes the Author field of the squashed commit. This confuses the Eclipse ECA check, which is a technical problem, but arguably there is also a philosophical issue, in that it potentially mis-attributes commits (especially if authors have multiple "personae" - it appears that squash and merge overrides whatever username / email an author has configured for a particular repository, and just uses the account default, see the discussion here:
https://github.com/isaacs/github/issues/1368).
This is not a (major) problem for the core committers (we don't need to sign off our commits so the ECA check almost never fails for us), but it is causing headaches when accepting PRs from third party contributors.
As an example, let's look at a recent pull request by Allessandro Bollini (
https://github.com/eclipse/rdf4j/pull/2571). This was properly done, with several commits all nicely signed off. Each individual commit on that PR had the correct Author field, that corresponded with the sign-off:
❯ git log -2
commit 60c0cea074156c3cd3a766ad0f7cd2bffc56e164 (HEAD -> GH-2564-triple-equals-contract, knoan/GH-2564-triple-equals-contract)
Date: Sat Oct 3 11:47:33 2020 +0200
GH-2564 Turn warning on Triple into an @implNote
commit 55b52b5f9fe793bd66d688a8ca1f43c8c5b819ca
Date: Fri Oct 2 08:57:58 2020 +0200
GH-2564 Specify Triple.equals()/.hashCode() implementation
However, when we squash-merged this into master, the git log for the squashed commit looks like this:
commit 219ed1d21626f7b8e2e903cdd945826e2baaccdf
Date: Wed Oct 7 02:13:48 2020 +0200
Specify Triple.equals()/.hashCode() implementation (#2571)
* GH-2564 Specify Triple.equals()/.hashCode() implementation
* GH-2564 Turn warning on Triple into an @implNote
Note the author field: it's suddenly been changed to a 'noreply.github.com' address, and therefore no longer corresponds to the signoff. This is why the current master-develop sync PR (
https://github.com/eclipse/rdf4j/pull/2581) is blocked (I have asked the webmaster to make the ECA check non-required so that we can at least merge this, but clearly there is a wider issue here).
We can argue over the usefulness of signoffs and IP tracking using the Author field, but at the end of the day we need a record of these things, and I can accept the EMO's insistence on proper attribution. The attribution rewriting that Github does when squash-merging is clearly at odds with the goal to have attributable commits to track intellectual property.
The author of a merge-commit, incidentally, is always the one pushing the merge button (rather than the PR author, as in the case of a squash-merge).
At this point in time I'm fed up with having to go back and forth on this, and I feel that to keep things simple, I should give up on the "clean linear history" goal and just go back to only using merge-commits. I will still want to insist on some manual squashing to keep the history tidy and commit messages meaningful, but I won't require a single commit per PR.
Let me know if you have any objections to this, if not I'll take this forward and do the necessary changes to our repo setup and our workflow documentation. Your day to day won't be too much affected other than that you may sometimes need to manually squash your PRs to tidy up before a merge.
Yours in exasperation,
Jeen
