I think my opinion is unpopular but I don't think [BEAM-XXXX] is important to have in the *subject* line of a commit message, but putting it in the body of the commit message is fine and could even be better.
Mostly I want to know the actual change and I would save all subject line characters for making that clear. If I wonder about the details, I read the rest of the commit message, so having the Jira number in the extended description is helpful for digging in. In fact I'd prefer to have the full URL in the body so it is even easier to immediately jump to it. One way to enforce that someone has thought about something in a commit is to force a line like JIRA=... in the body of the commit message. Then if it is just a little cleanup it could be JIRA=n/a. This would block cleanup commits unless someone strangely added that to them. This is very automation-friendly, too. Is there a way to do this without requiring an offline script? Kenn On Fri, Mar 22, 2019 at 9:38 AM Brian Hulette <bhule...@google.com> wrote: > It sounds like maybe we've already reached a consensus that committers > just need to be more vigilant about squashing fixup commits, and hopefully > automate it as much as possible. But I just thought I'd also point out how > the arrow project handles this as a point of reference, since it's kind of > interesting. > > They've written a merge_arrow_pr.py script [1], which committers run to > merge a PR. It enforces that the PR has an associated JIRA in the title, > squashes the entire PR into a single commit, and closes the associated JIRA > with the appropriate fix version. > > As a result, the commit granularity is equal to the granularity of PRs, > JIRAs are always linked to PRs, and the commit history on master is > completely linear (they also have to force push master after releases in > order to maintain this, which is the subject of much consternation and > debate). > > The simplicity of 1 PR = 1 commit is a nice way to avoid the manual > intervention required to squash fixup commits and enforce that every commit > has passed CI, but it does have down-sides as Etienne already pointed out. > > Brian > > [1] https://github.com/apache/arrow/blob/master/dev/merge_arrow_pr.py > > > On Fri, Mar 22, 2019 at 7:46 AM Mikhail Gryzykhin < > gryzykhin.mikh...@gmail.com> wrote: > >> I agree with keeping history clean. >> >> Although, Small commits like address PR comments are useful during review >> process. They allow reviewer to see only new changes, not review whole diff >> again. Best to squash then before/on merge though. >> >> On Fri, Mar 22, 2019, 07:34 Ismaël Mejía <ieme...@gmail.com> wrote: >> >>> > I like the extra delimitation the brackets give, worth the two extra >>> > characters to me. More importantly, it's nice to have consistency, and >>> > the only way to be consistent with the past is leave them there. >>> >>> My point with the brackets is that we are 'getting close' to 10K issue >>> so we will then have 3 chars less, probably it does not change much >>> but still. >>> >>> On Fri, Mar 22, 2019 at 3:19 PM Robert Bradshaw <rober...@google.com> >>> wrote: >>> > >>> > On Fri, Mar 22, 2019 at 3:02 PM Ismaël Mejía <ieme...@gmail.com> >>> wrote: >>> > > >>> > > It is good to remind committers of their responsability on the >>> > > 'cleanliness' of the merged code. Github sadly does not have an easy >>> > > interface to do this and this should be done manually in many cases, >>> > > sadly I have seen many committers just merging code with multiple >>> > > 'fixup' style commits by clicking Github's merge button. Maybe it is >>> > > time to find a way to automatically detect these cases and disallow >>> > > the merge or maybe we should reconsider the policy altogether if they >>> > > are people who don't see the value of this. >>> > >>> > I agree about keeping our history clean and useful, and think those >>> > four points summarize things well (but a clarification on fixup >>> > commits would be good). >>> > >>> > +1 to an automated check that there are many extraneous commits. >>> > Anything the person hitting the merge button would easily see before >>> > doing the merge. >>> > >>> > > I would like to propose a small modification to the commit title >>> style >>> > > on that guide. We use two brackets to enclose the issue id, but that >>> > > really does not improve much the readibility and uses 2 extra spaces >>> > > of the already short space title, what about getting rid of them? >>> > > >>> > > Current style: "[BEAM-XXXX] Commit title" >>> > > Proposed style: "BEAM-XXXX Commit title" >>> > > >>> > > Any ideas or opinons pro/con ? >>> > >>> > I like the extra delimitation the brackets give, worth the two extra >>> > characters to me. More importantly, it's nice to have consistency, and >>> > the only way to be consistent with the past is leave them there. >>> > >>> > > On Fri, Mar 22, 2019 at 2:32 PM Etienne Chauchot < >>> echauc...@apache.org> wrote: >>> > > > >>> > > > Thanks Alexey to point this out. I did not know about these 4 >>> points in the guide. I agree with them also. I would just add "Avoid >>> keeping in history formatting messages such as checktyle or spotless fixes" >>> > > > If it is ok, I'll submit a PR to add this point. >>> > > > Le vendredi 22 mars 2019 à 11:33 +0100, Alexey Romanenko a écrit : >>> > > > >>> > > > Etienne, thanks for bringing this topic. >>> > > > >>> > > > I think it was already discussed several times before and we have >>> finally came to what we have in the current Committer guide “Granularity of >>> changes" [1]. >>> > > > >>> > > > Personally, I completely agree with these 4 rules presented there. >>> The main concern is that all committers should follow them as well, >>> otherwise we still have sometimes a bunch of small commits with >>> inexpressive messages (I believe they were added during review process and >>> were not squashed before merging). >>> > > > >>> > > > In my opinion, the most important rule is that every commit should >>> be atomic in terms of added/fixed functionality and rolling it back should >>> not break master branch. >>> > > > >>> > > > [1] >>> https://beam.apache.org/contribute/committer-guide/#pull-request-review-objectives >>> > > > >>> > > > >>> > > > On 22 Mar 2019, at 10:16, Etienne Chauchot <echauc...@apache.org> >>> wrote: >>> > > > >>> > > > Hi all, >>> > > > It has already been discussed partially but I would like that we >>> agree on the commit granularity that we want in our history. >>> > > > Some features were squashed to only one commit which seems a bit >>> too granular to me for a big feature. >>> > > > On the contrary I see PRs with very small commits such as "apply >>> spotless" or "fix checkstyle". >>> > > > >>> > > > IMHO I think a good commit size is an isolable portion of a >>> feature such as for ex "implement Read part of Kudu IO" or "reduce >>> concurrency in Test A". Such a granularity allows to isolate problems >>> easily (git bisect for ex) and rollback only a part if necessary. >>> > > > WDYT about: >>> > > > - squashing non meaningful commits such as "apply review comments" >>> (and rather state what they do and group them if needed), or "apply >>> spotless" or "fix checkstyle" >>> > > > - trying to stick to a commit size as described above >>> > > > >>> > > > => and of course update the contribution guide at the end >>> > > > ? >>> > > > >>> > > > Best >>> > > > Etienne >>> > > > >>> > > > >>> >>
