Hi Steve, I realise that sign-offs in 3 adds a bit of overhead. I think it is something we should consider phasing in /if/ we think it is a good idea. I feel much more strongly about the attribution tags (Reported-by/Reviewed-by/Suggested-by) since they acknowledge people for their work and ideas.
What I like about the sign-off chain is that it provides some information about potential merge/rebase points. The support for forking [1] is a good example of a “complicated” chain of events. That particular patch first lived in my public git repo from when I was a PhD student, it was then rebased by Sascha, and finally (rebased and) published externally again by me (now from ARM). I plan to enforce it internally to track how we release code, but I don’t expect anyone else to use it at the moment. Sign-offs are can be added automatically by git when you commit your code (or amend a commit) using a command line switch. Adding links to review requests is a good idea, but it doesn’t acknowledge reviewers. The problem is that the review site is likely to disappear at some point, but the revision history lives “for ever”, so acknowledgements and tracking must live in the commits as well. Just as a side note, gerrit adds both the reviewed-by tags and reviewed-on (URL to review request) information automatically in our setup. For example (from a real commit): Change-Id: I6d2af1d666ace57124089648ea906f6c787ac63c Signed-off-by: Andreas Sandberg <[email protected]> Reviewed-on: http://[URL HERE]/[REQUEST ID] Reviewed-by: Nikos Nikoleris <[email protected]> Reviewed-by: Gabor Dozsa <[email protected]> Tested-by: [AUTOMATION USER] We typically strip the Tested-by and Reviewed-on tags before publishing patches externally since they are only really useful internally. However, if we would use gerrit externally as well, these would automatically link commits to a review request. Another nice thing in this setup is that gerrit automatically enforces that no changes are made after the review request since it automates committing the code to the repo. //Andreas [1] https://github.com/gem5/gem5/commit/a91c1e69a880dd6eec3cc980801ea18ddcbe7c3 1 On 12/05/2016, 23:25, "gem5-dev on behalf of Steve Reinhardt" <[email protected] on behalf of [email protected]> wrote: >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/12X3YHGJShUS34sorQJIvcr5NUssZcmMIJovD9 >>I >> > >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 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
