Hi Andreas, We discussed this internally today and the general feeling here is that the simple formatting changes (1 and 2 on your list) seem fine. Item 3 is OK as a recommendation/long-term goal but isn't something we'd want to enforce until we're all in the position where the insertion of these tags can be more automated.
Also, this reminds me that I had earlier suggested adding links from the commit messages back to the reviewboard entries. If we had such links, then some of the reviewed-by/signed-off-by info would be redundant, and we'd have the added benefit of being able to go back and look at the review discussion, which in a couple of cases has been really valuable in understanding why we allowed X to happen (or figuring out whether we noticed X and allowed it to happen or just didn't even notice X at the time). I don't know what the equivalent thing would be for a git-based workflow though. Steve On Thu, May 12, 2016 at 8:52 AM Jason Lowe-Power <[email protected]> wrote: > Hi Andreas, > > Thanks for the clarification. Point taken for "Reported-by" :). > > Jason > > On Thu, May 12, 2016 at 9:50 AM Andreas Sandberg <[email protected] > > > wrote: > > > Hi Jason, > > > > At least git keeps track of both the patch author and who pushes a patch. > > The signed-off-by chain would also fill this function. These tags are > > typically used (some organisations may enforce stricter rules locally) as > > statement by the developer saying “this change is technically sound and > > I’m allowed to commit it”. Similarly, whoever pushes the patch using > > today’s system would sign off the patch since they are effectively > > forwarding it through their tree. > > > > As for checking line breaks, I don’t really know if there is a good tool > > for that. Gerrit definitely shows a read line at column 75 when reviewing > > patches. Emacs seems to do “the right thing” when pressing M-q. Don’t > know > > about VI though. > > > > We could/should also write a commit message hook that enforces formatting > > similar to how we enforce code style. I’m tempted to offer my services > > here, but I currently have a lot of other projects going and I’m not sure > > if I could follow up on that commitment within a reasonable time frame. > > > > I’m not sure I agree with you that we need a bug tracking system for > > "Reported-by” to be useful. It would be useful for cases where someone on > > one of the mailinglists points out an issue and someone else fixes it. > > > > Cheers, > > Andreas > > > > On 12/05/2016, 15:18, "gem5-dev on behalf of Jason Lowe-Power" > > <[email protected] on behalf of [email protected]> wrote: > > > > >Hi Andreas, > > > > > >The updates to the character limits and formatting changes sound good to > > >me. Is there any way to make it easy to verify my commit messages meet > > >these requirements? My current approach is "ehhh, this looks like 70 > > >characters or so...". Maybe git has a good way to do this? > > > > > >As far as the tags go, "Reviewed by" sounds great to me. > > > > > >I'm not sure about the "Signed-off-by", though. Currently we don't have > > >any > > >process for who is required to sign off on a patch before it is > committed. > > >Though, I think this was proposed in this preliminary governance > document > > >we never actually "ratified": > > > > > > https://docs.google.com/document/d/12X3YHGJShUS34sorQJIvcr5NUssZcmMIJovD9I > > >3w0Ns/edit?usp=sharing > > >Similarly, we don't really have a method to report bugs either, so I'm > not > > >sure if "Reported-by" is needed, unless we implement (read: *use*) a > > >reporting system. > > > > > >Maybe we also need a "Pushed-by" or is this the same thing as > > >"Signed-off-by"? > > > > > >Thanks for bringing this up! > > > > > >Jason > > > > > >On Thu, May 12, 2016 at 9:08 AM Andreas Sandberg > > ><[email protected]> > > >wrote: > > > > > >> I just realised that I forgot a useful tag, we should probably add a > > >> “Reported-by:” to acknowledge people who report bugs. > > >> > > >> //Andreas > > >> > > >> On 12/05/2016, 14:37, "gem5-dev on behalf of Andreas Sandberg" > > >> <[email protected] on behalf of [email protected]> > > wrote: > > >> > > >> >Hi Everyone, > > >> > > > >> >I¹d like to open up a discussion around the gem5 commit message style > > >> >guide. In particular, I¹d like to make life better for users of > > >>git-baesd > > >> >tools, improve tracking, and acknowledge reviewers. > > >> > > > >> >The current style guide[1] mandates this: > > >> > > > >> > * The first line of a commit message is a summary line > > >> > * Subsequent lines provide a detailed description > > >> > > > >> > * The summary line starts with a series of keywords separated by > > >>commas > > >> >followed by a colon and a short patch description. > > >> > * The summary line can¹t be longer than 65 characters. > > >> > > > >> > * The detailed description is wrapped to fit in 80 columns. > > >> > > > >> > > > >> >I would propose to make three changes: > > >> > > > >> >1. Limit detailed lines to 75 characters > > >> > > > >> >A lot of tools (especially git-based tools) nowadays seem to assume > > >>that > > >> >the detailed description is wrapped at 75 characters or less. The Git > > >>Book > > >> >and GitHub suggest ³72 characters or so² [2, 4], Linux mandates > ³70-75 > > >> >characters² [3]. I¹d suggest that that we limit summary lines to 75 > > >> >characters. This is in practice the upper limit imposed by the kernel > > >>and > > >> >would work nicely with the way git indents log lines. > > >> > > > >> >2. Add a blank line between the summary line and the detailed > > >>description > > >> > > > >> >This seems to be suggested by Linux, GitHub and the Git Book. > > >> > > > >> > > > >> >3. Additional tracking and acknowledgements > > >> > > > >> >A long-term change that I¹d like to see is adding more tracking to > > >>commit > > >> >messages. In particular, I would like to introduce these tags (see > [3] > > >>for > > >> >a description of how they are used in Linux): > > >> > > > >> > * Reviewed-by: This would be used to acknowledge the work of > > >>reviewers. > > >> >We already use this internally and that have gone through internal > > >>review > > >> >within ARM will have these tags. > > >> > > > >> > * Signed-off-by: This tag tells other developers that the patch > author > > >> >has the right to submit this work. Similarly, people in the delivery > > >>path > > >> >of a path add a this tag to say to indicate that they have received > it > > >>in > > >> >good faith and forwarded it. See [5] for a description of how the > > >>kernel > > >> >uses it. Any modifications (rebasing) would be described within > > >>brackets. > > >> >For example: > > >> > > > >> > Signed-off-by: Foo Bar <. . .> > > >> > [Rebased to work with latest gem5] > > >> > Signed-off-by: Andreas Sandberg <. . .> > > >> > > > >> > > > >> >Another good example would be the fork patches [6]. These were > written > > >>by > > >> >me before I joined ARM, rebased by Sascha Bischoff, and finally > pushed > > >>by > > >> >me. > > >> > > > >> >Cheers, > > >> >Andreas > > >> > > > >> >[1] http://www.gem5.org/Submitting_Contributions > > >> > > > >> >[2] https://git-scm.com/book/ch5-2.html > > >> > > > >> >[3] > > >> > > >>> > > http://lxr.free-electrons.com/source/Documentation/SubmittingPatches#L60 > > >>>0 > > >> > > > >> >[4] https://github.com/blog/926-shiny-new-commit-styles > > >> > > > >> >[5] > > >> > > >>> > > http://lxr.free-electrons.com/source/Documentation/SubmittingPatches#L40 > > >>>9 > > >> > > > >> >[6] > > >> > > > >> > > >> > > > https://github.com/gem5/gem5/commit/738d71f6a93e2da9ef5bb1490f2b94f954173 > > >>5 > > >> >e > > >> >3 > > >> > > > >> >IMPORTANT NOTICE: The contents of this email and any attachments are > > >> >confidential and may also be privileged. If you are not the intended > > >> >recipient, please notify the sender immediately and do not disclose > the > > >> >contents to any other person, use it for any purpose, or store or > copy > > >> >the information in any medium. Thank you. > > >> > > > >> >_______________________________________________ > > >> >gem5-dev mailing list > > >> >[email protected] > > >> >http://m5sim.org/mailman/listinfo/gem5-dev > > >> > > >> IMPORTANT NOTICE: The contents of this email and any attachments are > > >> confidential and may also be privileged. If you are not the intended > > >> recipient, please notify the sender immediately and do not disclose > the > > >> contents to any other person, use it for any purpose, or store or copy > > >>the > > >> information in any medium. Thank you. > > >> _______________________________________________ > > >> gem5-dev mailing list > > >> [email protected] > > >> http://m5sim.org/mailman/listinfo/gem5-dev > > >> > > >-- > > > > > >Jason > > >_______________________________________________ > > >gem5-dev mailing list > > >[email protected] > > >http://m5sim.org/mailman/listinfo/gem5-dev > > > > IMPORTANT NOTICE: The contents of this email and any attachments are > > confidential and may also be privileged. If you are not the intended > > recipient, please notify the sender immediately and do not disclose the > > contents to any other person, use it for any purpose, or store or copy > the > > information in any medium. Thank you. > > _______________________________________________ > > gem5-dev mailing list > > [email protected] > > http://m5sim.org/mailman/listinfo/gem5-dev > > > -- > > Jason > _______________________________________________ > gem5-dev mailing list > [email protected] > http://m5sim.org/mailman/listinfo/gem5-dev > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
