Possible amendment to the cherry-picking rules
Today it came to my attention that not all Ceph developers agree with the following cherry-picking rule: "if a commit could not be cherry-picked from master, the commit message must explain why that was not possible" [1] [1] https://github.com/ceph/ceph/blob/master/SubmittingPatches-backports.rst#che... Now, I (Nathan) am the one who wrote these rules down, but I'm not their author. These rules are a codification of a set of best practices I "inherited" from Loic. Although I hesitate to speak on his behalf, I don't think he's around here anymore, so I'll just go ahead and present what I think is the rationale for this particular rule. In the past, regressions often happened because bugs got fixed directly in a stable branch, but not in master. Later, after a new major stable release was split off from master and users upgraded their clusters to it, BOOM the bugs were back! Of course, nobody initially knew why, but it was clear that the bug was a regression. Therefore, forensic investigations of the git history were undertaken to find the answer to the question: "which commit fixed this bug in N-1 and why is that commit not in N?". One possible tactic in such an investigation is to find all commits in the N-1 stable branch (which does not exhibit the bug) that aren't cherry-picks, but potentially should have been. One of these might be the fix, but which one? Some bugs have to be fixed directly in a stable branch: they cannot be cherry-picked from master for any number of valid reasons. So, in our hypothetical forensic investigation, we are faced with the necessity of distinguishing these "good" direct bug-fixing commits from "bad" ones which should have been cherry-picks, but are not. But how to make that distinction when the commit messages themselves are silent on the question of why they aren't cherry picks? That, I believe, is where this rule came from. Nowadays, it would seem that this type of forensic investigation is rarely undertaken. BUT let us ask ourselves, could that be because (1) we have these cherry-picking rules and (2) they are - for the most part - enforced? Anyway, I thought I'd bring the matter up here to in the hopes of finding a consensus on whether this rule should stand as-is or be revised. I don't relish being in a position of enforcing a rule that the leads (and the developer community as a whole) don't understand or agree with. Thanks, Nathan
Hello Nathan, Thanks for starting the discussion on this list. On Thu, Sep 24, 2020 at 8:57 AM Nathan Cutler <ncutler@suse.com> wrote:
Today it came to my attention that not all Ceph developers agree with the following cherry-picking rule:
"if a commit could not be cherry-picked from master, the commit message must explain why that was not possible" [1]
[1] https://github.com/ceph/ceph/blob/master/SubmittingPatches-backports.rst#che...
Now, I (Nathan) am the one who wrote these rules down, but I'm not their author. These rules are a codification of a set of best practices I "inherited" from Loic. Although I hesitate to speak on his behalf, I don't think he's around here anymore, so I'll just go ahead and present what I think is the rationale for this particular rule.
In the past, regressions often happened because bugs got fixed directly in a stable branch, but not in master. Later, after a new major stable release was split off from master and users upgraded their clusters to it, BOOM the bugs were back! Of course, nobody initially knew why, but it was clear that the bug was a regression. Therefore, forensic investigations of the git history were undertaken to find the answer to the question: "which commit fixed this bug in N-1 and why is that commit not in N?".
One possible tactic in such an investigation is to find all commits in the N-1 stable branch (which does not exhibit the bug) that aren't cherry-picks, but potentially should have been. One of these might be the fix, but which one?
Some bugs have to be fixed directly in a stable branch: they cannot be cherry-picked from master for any number of valid reasons. So, in our hypothetical forensic investigation, we are faced with the necessity of distinguishing these "good" direct bug-fixing commits from "bad" ones which should have been cherry-picks, but are not. But how to make that distinction when the commit messages themselves are silent on the question of why they aren't cherry picks? That, I believe, is where this rule came from.
Nowadays, it would seem that this type of forensic investigation is rarely undertaken. BUT let us ask ourselves, could that be because (1) we have these cherry-picking rules and (2) they are - for the most part - enforced?
From what I've seen myself, we strictly enforce this rule. I agree with the rationale you've shared above. Any PR against a stable branch that includes (OR excludes!) commits or fixes not present on master must explain why. -- Patrick Donnelly, Ph.D. He / Him / His Principal Software Engineer Red Hat Sunnyvale, CA GPG: 19F28A586F808C2402351B93C3301A3E258DD79D
Today it came to my attention that not all Ceph developers agree with the following cherry-picking rule:
"if a commit could not be cherry-picked from master, the commit message must explain why that was not possible" [1]
[1] https://github.com/ceph/ceph/blob/master/SubmittingPatches-backports.rst#che...
From what I've seen myself, we strictly enforce this rule. I agree with the rationale you've shared above. Any PR against a stable branch that includes (OR excludes!) commits or fixes not present on master must explain why.
Thanks for your quick response, Patrick! Would you agree, then, to change the rule to allow the explanation of "why" to be done outside of the commit messages themselves? My concern is that by intentionally not including the explanation in the commit message, we are effectively withholding the explanation from future users of the git history. To put it another way, folks who are unfortunate enough to be involved in the kind of forensic examination I described will not benefit from explanations that are offered up in a PR or tracker issue. Nathan
On Thu, Sep 24, 2020 at 12:16 PM Nathan Cutler <ncutler@suse.com> wrote:
Today it came to my attention that not all Ceph developers agree with the following cherry-picking rule:
"if a commit could not be cherry-picked from master, the commit message must explain why that was not possible" [1]
[1] https://github.com/ceph/ceph/blob/master/SubmittingPatches-backports.rst#che...
From what I've seen myself, we strictly enforce this rule. I agree with the rationale you've shared above. Any PR against a stable branch that includes (OR excludes!) commits or fixes not present on master must explain why.
Thanks for your quick response, Patrick! Would you agree, then, to change the rule to allow the explanation of "why" to be done outside of the commit messages themselves?
My concern is that by intentionally not including the explanation in the commit message, we are effectively withholding the explanation from future users of the git history. To put it another way, folks who are unfortunate enough to be involved in the kind of forensic examination I described will not benefit from explanations that are offered up in a PR or tracker issue.
Tracker tickets and PRs which are also linked to the git commit -- hardly a forensic examination. ;-)
Nathan _______________________________________________ Dev mailing list -- dev@ceph.io To unsubscribe send an email to dev-leave@ceph.io
-- Jason
On Thu, Sep 24, 2020 at 9:16 AM Nathan Cutler <ncutler@suse.com> wrote:
Today it came to my attention that not all Ceph developers agree with the following cherry-picking rule:
"if a commit could not be cherry-picked from master, the commit message must explain why that was not possible" [1]
[1] https://github.com/ceph/ceph/blob/master/SubmittingPatches-backports.rst#che...
From what I've seen myself, we strictly enforce this rule. I agree with the rationale you've shared above. Any PR against a stable branch that includes (OR excludes!) commits or fixes not present on master must explain why.
Thanks for your quick response, Patrick! Would you agree, then, to change the rule to allow the explanation of "why" to be done outside of the commit messages themselves?
Yes, I think having the discussion in the PR is also suitable. Otherwise where in the commit history would you document a missing commit form a series in a backport.
My concern is that by intentionally not including the explanation in the commit message, we are effectively withholding the explanation from future users of the git history. To put it another way, folks who are unfortunate enough to be involved in the kind of forensic examination I described will not benefit from explanations that are offered up in a PR or tracker issue.
GitHub/tracker discussions are all official metadata associated with the git history. It's not feasible to make git (commits) the SSOT. (Maybe when git adds changesets/reviews/issues we can get to that point. :) -- Patrick Donnelly, Ph.D. He / Him / His Principal Software Engineer Red Hat Sunnyvale, CA GPG: 19F28A586F808C2402351B93C3301A3E258DD79D
participants (3)
-
Jason Dillaman
-
Nathan Cutler
-
Patrick Donnelly