Can we drop the Gerrit Change-IDs requirement?

[BobbyRBruce]

When migrating from Gerrit to Github we felt we needed to keep Change-IDs in commit messages to keep our mainline gem5 Github repo compatible with those who use Gerrit to develop gem5. However, I must confess we probably made things worse for Gerrit users in trying to keep this going. In enforcing commit messages to contain change IDs we accidentally introduced the following problems:

1. When Squash merges were allowed, and we didn't have a good policy on the default commit message for this merge, squashed commit could exist containing multiple change IDs. Gerrit can't process these.
2. Since we changed using traditional mergers we have defaulted the merge commits header and content to the PR title and description. Unfortunately, the PR header and description, for single commit PRs, defaults to that of the commit. Ergo, when merged both the merge commit and the commit have the same ChangeID. I don't know how Gerrit understands this (perhaps sees it as a cherry-pick?) but it's not useful or good information.

(See discussion #261 for a discussion about GitHub Merge policy).

In addition, now that we're being a bit more careful on preventing (2.) by removing Change-IDs from PR descriptions manually, the merge commit does not have a Change-ID, so our policy of every commit in gem5 doesn't hold.

On our end keeping Gerrit IDs in commits is causing the following pain-points:

1. To add a Gerrit Change-ID to something already contributed to gem5 via a PR involves rebasing your changes, which is a little annoying and daunting for new contributors.
2. GitHub has some nice functionality to add commits via the web interface. For example, reviewers can make suggestions in a review and these suggestions can be added as as commit to the PR in a single click. However, as we have a CI tests that makes sure all commits in a PR have a Change-ID, we can't really use this feature (side-note: This works best wish Squash Merges, see discussion Open discussion about gem5's PR Merge Policy: Should we use `Merge` `Squash` or `Rebase`? #261 for more on this).

My question to the community is, can we drop this requirement? I have created PR #315 to remove it. For those that use Gerrit I believe the following is true: Commits made through GitHub without a change-id are fine and don't make a Gerrit system unworkable. It just means that you must merge the GitHub repo directly with the repo Gerrit uses and not via the Gerrit system (which is fine because the gem5 GitHub patches have already been reviewed). On the flip-side, commits may still contain Change-IDs and they can be enforced for those who contribute via a Gerrit systems. If these patches are then contributed to the GitHub gem5 repo, GitHub will not complain.

[mattsinc]

@abmerop : I thought you had said previously on Github (can't seem to find where) that this change would break Gerrit compatibility on your end at AMD. Am I misremembering?

[abmerop]

It doesn't break things, it just makes it a bit harder to merge. However, this is not the only problem trying to pull from an upstream github repo to a gerrit repo, so removing the Change-ID would not be a huge issue.

[andysan]

@giactra and @rjc-arch, you have probably done more cherry-picks through gerrit recently. What are your thoughts on this?

In general, my experience is that Gerrit gets more upset if you have multiple commits with the same Change-ID or commits with multiple Change-IDs. Not having a change ID makes tracking of logical changes (e.g., for backporting) a bit tricky since the maintainer tooling in the main repo uses change IDs to track changes.

[rjc-arch]

Hi Bobby. I appreciate the difficulties you have mentioned above.

From my point of view, more than aiding the continued use of Gerrit (which is also of benefit), we have found the Change-IDs useful for tracking patches as they are ported between branches. While porting changes between branches often requires modifying the code, and will inevitably result in a different commit hash, the Change-ID can be used to track a 'logical change' between branches. Some existing tooling already uses this feature; for example util/maint/create_patches.sh.

For the purpose of tracking 'logical changes' it isn't a big problem to have no Change-ID in merge commits, or (to a lesser extent) to have multiple Change-IDs in a squash commit (although the existing tooling will need to be updated to cope with these cases).

For these reasons I would prefer us to keep the Change-ID requirement if possible. However I accept the problems you mentioned above, as well as @andysan's point about Gerrit not liking multiple Change-IDs in a single commit.

[BobbyRBruce]

I understand in git it's possible to create "duplicate commits" with different SHA values, but isn't this alleviated by merging branches? I can only see this happening in cases where you rebase/cherry-pick. And even then, in the case of the latter, where you keep the old rebased branch around rather than just get rid of it.

I can only really think of two scenarios:

1. Branch X is merged into Branch Y : commit revs are preserved. No duplicate commits to consider. No tracking problems.
2. Branch X is rebased on top of Branch Y (of note: this is effectively what a cherry picking is so this case covers that): This is where you can have duplicate commits with different revisions on different branches. My argument here is: if Branch X is short lived (i.e., its purpose is solely to implement the feature, be added to the main Y branch, then cease to be used again), then Branch X should be destroyed after it's added to Branch Y. Ergo, no duplicates. If, on the other hand, branch X is long lived and will exist after being added to Y for more development (like develop is to stable in gem5), then isn't it more sensible to use merges? Rebasing changes continuously from X to Y in this case seems like it may cause problems more generally and doesn't seem to provide many benefits outside of a desire to preserve a linear branch.

That being said, it's ok with me accommodating this caase. Having something in the commit message to lookup can be done in a way that, in the worst case, doesn't harm anything. Perhaps this ID can: A) be something which isn't required or strictly enforced; B) Independent from any code review system; and C) conveys some grander information rather than just being a hash.

What I miss from Gerrit is a link back to the Gerrit review page in the Commit message body (Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/60516). I've been wondering if there's anything we can do to automatically add Reviewed-on to our git commits which links back to the PR that created them (E.g., something like Reviewed-on: https://github.com/gem5/gem5/pull/4)**. I've not came up with any concrete proposal on this yet. The best answer I have would be a GitHub action that, upon a merge of a PR, would update the commit message with it to add this. However this would alter the commits and therefore change their SHAs. I don't know what the effects of this are (maybe nothing) but it worries me so i haven't moved forward with trying.

[** Note: this wouldn't give each commit a unique ID by itself as there can be multiple commits in a PR. To fix this we could add something like PR-Commit-No: which simply gave an integer indicating which commit it is in the chain of commits in the PR --- PR-Commit-No: 1 for the first commit PR-Commit-No: 2 for the second commit, etc. It wouldn't mean very much but would mean PR-Commit-No + Reviewed-on gave a unique ID for commits made through the Gerrit Repo]

[rjc-arch]

Hi Bobby,

Your scenarios make sense for managing development branches, but I was thinking of private branches that represent forks of the repository. For example, some industry organizations likely maintain private branches mirroring the gem5 releases, but containing features that are not ready for public release. Keeping these forks/branches up to date often requires porting changes from the mainline gem5 branches, and also between the private branches. In these cases the Change-IDs are invaluable for tracking which logical changes have been included in each branch, and which have not.

If we do decide to remove the Change-Id requirement, I like your compromise idea of having a separate, "non-mandatory but strongly recommended" identifier. The format could be the same as the existing 'Change-Id: ' but with a new name (e.g. Gem5-Commit-Id). This would provide the same utility without causing any conflict within Gerrit.

As an aside, I also miss the links back to the review page, and also the ability to search Gerrit using the Change-Id. Again, having a unique reference to a logical change is very useful when referring back to a previous commit from a later commit's message.

Thanks again for taking our needs into consideration!