Hi Andreas, I think you're reading more negativity into my emails than actually exists :). I'm not opposed to skipping review for trivial changes.
There is a secondary issue here in that our commit process is not crisply defined. In fact I think the exception for "trivial changes" that was on the old wiki page seems to have gotten unintentionally lost in the editing process. Since you brought it up, I'll point out (as a very minor thing) that it's confusing to post trivial changes on RB because that creates the impression that reviews are being solicited; if it's truly a trivial change, the committer should just commit it. However, although this whole thread got initiated because of my experience trying to track down the review history of your commit, I raised the idea of RB URLs only because it's the Nth time I've seen a commit go by and had to go searching on RB to determine the review status, not because your patch was particularly egregious in making me want to do that in any way. Steve On Mon, Sep 7, 2015 at 7:52 AM Andreas Hansson <[email protected]> wrote: > Hi Steve, > > I considered this particular patch to fall in the 'trivial fix/update, > clean up, and/or documentation/clarirication addition'. The posting on RB > was merely an FYI. > > If you are suggesting we should never push anything without first going > through a review cycle, then let’s all agree to that. I personally > question the cost-benefit of doing so. > > Andreas > > On 07/09/2015 15:21, "gem5-dev on behalf of Steve Reinhardt" > <[email protected] on behalf of [email protected]> wrote: > > >I don't have any problem with the particular patch itself, which is why I > >didn't complain about it specifically. I was a bit thrown off by the > >non-standard process though. It was the effort required to determine that > >this patch had indeed followed a non-standard process (and it not being > >the > >first time I've seen a commit fly by and thought "did that really get > >adequately reviewed?") that kicked off this whole thread... > > > >Steve > > > >On Sun, Sep 6, 2015 at 8:03 AM Andreas Hansson <[email protected]> > >wrote: > > > >> Hi Steve, > >> > >> Do you have any concern with the patch? Given that it is a rather > >>obvious > >> fix (and a one-liner), I decided not to wait. > >> > >> Andreas > >> > >> On 04/09/2015 18:54, "gem5-dev on behalf of Steve Reinhardt" > >> <[email protected] on behalf of [email protected]> wrote: > >> > >> >To be concrete, Andreas, I saw your commit of the "Avoid setting > >> >markPending" changeset, and I thought "gee, I wonder how long that was > >>on > >> >reviewboard, and whether it got any ship-its, because I don't recall > >> >reviewing it". I was able to find it by searching my email for > >> >"markPending" to see the review request (and to find out that it had > >>only > >> >been posted for just over four days and had not received any ship-its, > >>or > >> >any reviews at all---but that's a separate issue). It would have been > >>much > >> >easier to answer my initial question if there had been a link in your > >> >commit message, and I can see the link being even more valuable when a > >> >long > >> >time has passed since the review request was posted, or if the > >>description > >> >doesn't contain any particularly unique terms to simplify a search. > >> > > >> >To answer Jason's question: it would be nice to have an automation > >>system > >> >where someone with sufficient permissions could just click on a patch > >>on > >> >reviewboard and the patch would automatically be committed; with that > >> >process in place, it doesn't seem like much of a stretch to include the > >> >URL > >> >in the commit message. I know Ali has discussed some other tools that > >> >enable this. The easiest path to this kind of functionality might > >>require > >> >migrating to something other than reviewboard. > >> > > >> >Steve > >> > > >> > > >> >On Fri, Sep 4, 2015 at 10:38 AM Andreas Hansson > >><[email protected]> > >> >wrote: > >> > > >> >> No strong opinion in either direction. I have not experienced a > >> >>situation > >> >> where it was needed, but I also do not think it is too much to ask. > >> >> > >> >> Andreas > >> >> > >> >> On 04/09/2015 18:34, "gem5-dev on behalf of Gutierrez, Anthony" > >> >> <[email protected] on behalf of [email protected]> > >> >>wrote: > >> >> > >> >> >I like that idea. Or at least the review number, for brevity's sake. > >> >> > > >> >> >-----Original Message----- > >> >> >From: gem5-dev [mailto:[email protected]] On Behalf Of > Steve > >> >> >Reinhardt > >> >> >Sent: Friday, September 04, 2015 10:32 AM > >> >> >To: gem5 Developer List > >> >> >Subject: [gem5-dev] including reviewboard URL in commit message? > >> >> > > >> >> >Hi everyone, > >> >> > > >> >> >Just a thought: should we require commit messages to include the > >>URL of > >> >> >the corresponding reviewboard posting? I think that would be > >>useful in > >> >> >tying the discussion to the commit. Apologies if someone suggested > >> >>this > >> >> >before and I missed it. > >> >> > > >> >> >Steve > >> >> >_______________________________________________ > >> >> >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. > >> >> > >> >> ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, > >> >> Registered in England & Wales, Company No: 2557590 > >> >> ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 > >> >>9NJ, > >> >> Registered in England & Wales, Company No: 2548782 > >> >> _______________________________________________ > >> >> 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. > >> > >> ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, > >> Registered in England & Wales, Company No: 2557590 > >> ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 > >>9NJ, > >> Registered in England & Wales, Company No: 2548782 > >> _______________________________________________ > >> 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. > > ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, > Registered in England & Wales, Company No: 2557590 > ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, > Registered in England & Wales, Company No: 2548782 > _______________________________________________ > 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
