TOOL-21469 Allow empty commits when cherry-picking in kernel repos

[palash-gandhi]

Problem

Recently, a merge commit was pushed to the linux-kernel-aws repo
because the author clicked the "Create a merge commit" option to
merge their PR. This caused the update-package jobs to fail with:

00:14:35.611  error: commit a63f70b93cad58e3b38e0718f2e25663855ffddc is a merge but no -m option was given.
00:14:35.611  fatal: cherry-pick failed

To fix this, we decided to fix the git history. But pushing to these repos requires
2 approvals on a PR. To open a PR, we had to create an empty commit. The resulting PR is: delphix/linux-kernel-aws#41

I manually started an update-package job to check if the history rewrite fixes the issue. It failed with:
http://ops.jenkins.delphix.com/job/linux-pkg/job/develop/job/update-package/job/linux-kernel-aws/9/console

00:14:33.891  The previous cherry-pick is now empty, possibly due to conflict resolution.
00:14:33.891  If you wish to commit it anyway, use:
00:14:33.891  
00:14:33.891      git commit --allow-empty
00:14:33.891  
00:14:33.891  and then use:
00:14:33.891  
00:14:33.891      git cherry-pick --continue
00:14:33.891  
00:14:33.891  to resume cherry-picking the remaining commits.
00:14:33.891  If you wish to skip this commit, use:
00:14:33.891  
00:14:33.891      git cherry-pick --skip
00:14:33.891  
00:14:33.891  On branch repo-HEAD
00:14:33.891  Cherry-pick currently in progress.
00:14:33.891    (run "git cherry-pick --continue" to continue)
00:14:33.891    (use "git cherry-pick --skip" to skip this patch)
00:14:33.891    (use "git cherry-pick --abort" to cancel the cherry-pick operation)
00:14:33.891  
00:14:33.891  nothing to commit, working tree clean
00:14:33.891  Error: failed command 'git cherry-pick ebaf51ee12c4be20aeaa6fd9b4fcafd61d2f06c6^..24afab62888cb3f09a8e0f1d82fadcf32d452786'

This was because cherry-picking an empty commit requires the --allow-empty option.

Solution

Add the --allow-empty option. To prevent such issues in the future, I started working on a change
to disallow rebase merges but I am still looking into if this is something that will work for us: https://github.com/delphix/github-infra/pull/615

Testing Done

TBA

[prakashsurya]

I have my doubts about this being a good idea.. can you add some information about why you think we want to do this?

[palash-gandhi]

I have my doubts about this being a good idea.. can you add some information about why you think we want to do this?

@prakashsurya I added some info

[prakashsurya]

I don't see any reason to have this empty commit in that repository:

> g show 24afab62888cb3f09a8e0f1d82fadcf32d452786
commit 24afab62888cb3f09a8e0f1d82fadcf32d452786 (aws/develop)
Author: Palash Gandhi <[email protected]>
Date:   Tue May 30 15:10:21 2023 -0700

    DLPX-86263 Remove merge commit from linux-kernel-aws

Why do we want/need it? Can't we just remove it, and then we don't need the changes in this PR.. I'm not sure why we added an empty commit, vs. updating that repository to just remove this empty commit, and the merge commit..?

[palash-gandhi]

I don't see any reason to have this empty commit in that repository:

> g show 24afab62888cb3f09a8e0f1d82fadcf32d452786
commit 24afab62888cb3f09a8e0f1d82fadcf32d452786 (aws/develop)
Author: Palash Gandhi <[email protected]>
Date:   Tue May 30 15:10:21 2023 -0700

    DLPX-86263 Remove merge commit from linux-kernel-aws

Why do we want/need it? Can't we just remove it, and then we don't need the changes in this PR.. I'm not sure why we added an empty commit, vs. updating that repository to just remove this empty commit, and the merge commit..?

The only reason to create it was to be able to open a PR in order to get approvals so that the corrected git history could be pushed. How would I go about removing a commit without a PR? I tried and the branch protections didn't let me:

$ git push -f origin HEAD:develop
Total 0 (delta 0), reused 0 (delta 0), pack-reused 0
remote: error: GH006: Protected branch update failed for refs/heads/develop.
remote: error: At least 2 approving reviews are required by reviewers with write access. 2 of 2 required status checks are expected.
To github.com:delphix/linux-kernel-aws
 ! [remote rejected]           HEAD -> develop (protected branch hook declined)
error: failed to push some refs to 'github.com:delphix/linux-kernel-aws'

[prakashsurya]

What we've done in the past when a merge commit is accidentally added.. is we rewrite the history, such that merge commit is no longer there.. open a PR with the new "fixed" history.. and then force push to merge the PR with git push -f..

Once you have the PR with approvals, the git push -f will work.. without the PR, it is (correctly) blocked..

[palash-gandhi]

What we've done in the past when a merge commit is accidentally added.. is we rewrite the history, such that merge commit is no longer there.. open a PR with the new "fixed" history.. and then force push to merge the PR with git push -f..

Once you have the PR with approvals, the git push -f will work.. without the PR, it is (correctly) blocked..

This is what I did, with the only caveat that each time I would open a PR without an empty commit, Github would automatically close it:
delphix/linux-kernel-aws#38
delphix/linux-kernel-aws#39
delphix/linux-kernel-aws#40

[prakashsurya]

I think, previously, in the process of removing the merge commit, we'd "rewrite" the commit we want to keep.. resulting in a different commit hash.. which lets the PR process work..

e.g. see here: delphix/linux-kernel-aws#43

I've removed the empty commit, and also did a git commit --amend on the commit we want to keep.. this changes that commit's hash, and then a PR can be created.. if we get reviews on this, then I should be able to git push -f to land it..

FWIW, here's the history for that PR:

> git log --oneline --no-decorate --graph | head
* a6bdc853000d4 DLPX-86177 Azure Accelerated networking broken because Mellanox drivers absent in kernel (#37)
* 3954819562635 DLPX-84906 Disable frame buffer drivers
* bde15809b85ed DLPX-84995 NFSD: Never call nfsd_file_gc() in foreground paths (#35)
* 82918ccd56776 DLPX-84985 target: iscsi: fix deadlock in the iSCSI login code (#30)
* 8225f91c81ac3 DLPX-84907 CVE-2022-3628 (#29)
* ae73b1650b067 DLPX-84469 Users unable to connect to CIFS mounts (#28)
* 0a2b1ebe587b5 DLPX-83701 Make function mnt_add_count() traceable (#24)
* 6921143650de5 target: login should wait until tx/rx threads have properly started. (#21)
* d5521fcac31e8 TOOL-16649 CONFIG_MD is needed on the buildserver (#22)
* e52eeb6151da5 DLPX-83442 Disable various kernel modules which we don't use (#20)

[palash-gandhi]

What are your reservations with this change? I don't mind discarding this PR but I am curious why we shouldn't allow cherry-picking empty commits

[prakashsurya]

but I am curious why we shouldn't allow cherry-picking empty commits

I'm not necessarily against it.. but I can't think of any good reason why we'd want to maintain empty commits in the history.. the one case that sparked this PR, IMO at least, isn't a good reason, since we can just remove the empty commit and avoid the problem entirely..

I think the empty commit just makes things more complicated.. e.g. if a future developer sees it, and is working on moving our patch stack to a new kernel version (like I just did, moving to the 5.15 kernel).. should they port it to the new kernel version? or drop it? why have it to begin with? IMO, it's unclear, and just makes things more difficult to reason about..

So then.. if the general rule (or, perhaps it's just my opinion) should be not to preserve empty commits in our patch stack.. I don't think this change is necessary..

But.. with that said, I'm OK landing this.. I'm just doubtful that having and preserving empty commits in our patch stack provides us any value.. if you think this change is useful, I don't feel strongly, I'm OK getting others to approve and land it..