Hi Andreas, I think we're in agreement here. I'm not opposed to the additional tags. If gerrit can automate this (and add in the link to the reviews) then that's a nice benefit of switching to that workflow. All I'm saying is let's not mandate it until we are all on such a workflow.
Steve On Fri, May 13, 2016 at 3:46 AM Andreas Sandberg <[email protected]> wrote: > 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 > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
