> On Jan. 7, 2016, 3:14 p.m., Steve Reinhardt wrote: > > Why was this patch committed? There are no ship-its, and there was no > > warning. I was complaining about minor things to buy myself a little time, > > and also because I hoped the changes between the similar files would be > > easier to detect/review if rename was used. I saw the updates on > > reviewboard and made a mental note that now I need to go back and > > re-review, and then next thing I know I see the changeset messages going > > by... > > Curtis Dunham wrote: > Hi Steve, > I committed it because of this comment you made on this RB: "Given that > it's clearly an improvement over the status quo (since it includes input from > pd-gem5) I don't have a problem with committing first and addressing any > issues I run across later, once the style issues are addressed." > > Our documentation says (http://gem5.org/Submitting_Contributions): > "If your patch has been on reviewboard for a while without getting any > reviews (or re-revires after you've posted changes), please email the > gem5-dev list. If you have commit access, it is fair to give warning via > email that you intend to commit the changes at some future date (e.g., a week > out from the date of the email) if you do not hear any objections. Please do > not simply commit a patch without giving warning on the gem5-dev list." > > I did precisely this; I made a plea for further review on the list on > December 10, stating a desire to commit on December 18, and this was the only > feedback I received, and to reiterate, the feedback we did get didn't sound > much like an objection.
It's not a big deal; my comment about being OK with submitting first and fixing later does still apply. It just seemed very odd that you posted an update to reviewboard and left it up for all of 45 minutes before you committed. Why even bother to post the update if you're not going to give people time to look at it? I'm familiar with the rule you quote. I know it's not clearly stated there, but to me, getting a comment that's not an explicit "ship it" is sufficient to reset the process. Also once you've posted an update you're no longer in the situation that you've gone "a while" without any "re-reviews after posting changes". I'm not saying you clearly broke the rule, just explaining how my interpretation makes it look that way to me :). Ironically if you had just committed the code on the 18th before I had time to look at it, I would not have any grounds for complaint... Again, not a big deal, just a misunderstanding. In general though I would request that people be conservative about assuming things are ready to commit if patches have been recently updated and/or there are outstanding comments w/o explicit ship-its. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3228/#review7830 ----------------------------------------------------------- On Jan. 7, 2016, 1:58 p.m., Curtis Dunham wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3228/ > ----------------------------------------------------------- > > (Updated Jan. 7, 2016, 1:58 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Distributed gem5 is the result of the convergence effort between multi-gem5 > and pd-gem5 (from Univ. of Wisconsin). It relies on the base multi-gem5 > infrastructure for packet forwarding, synchronisation and checkpointing but > combines those with the elaborated network switch model from pd-gem5. > > > Diffs > ----- > > src/dev/net/Ethernet.py 57c340f947c719a5acc3867037d51829c3967671 > src/dev/net/SConscript 57c340f947c719a5acc3867037d51829c3967671 > src/dev/net/etherpkt.hh 57c340f947c719a5acc3867037d51829c3967671 > src/dev/net/etherpkt.cc 57c340f947c719a5acc3867037d51829c3967671 > src/dev/net/multi_etherlink.hh 57c340f947c719a5acc3867037d51829c3967671 > src/dev/net/multi_etherlink.cc 57c340f947c719a5acc3867037d51829c3967671 > src/dev/net/multi_iface.hh 57c340f947c719a5acc3867037d51829c3967671 > src/dev/net/multi_iface.cc 57c340f947c719a5acc3867037d51829c3967671 > src/dev/net/multi_packet.hh 57c340f947c719a5acc3867037d51829c3967671 > src/dev/net/multi_packet.cc 57c340f947c719a5acc3867037d51829c3967671 > src/dev/net/tcp_iface.hh 57c340f947c719a5acc3867037d51829c3967671 > src/dev/net/tcp_iface.cc 57c340f947c719a5acc3867037d51829c3967671 > src/sim/global_event.hh 57c340f947c719a5acc3867037d51829c3967671 > src/sim/initparam_keys.hh PRE-CREATION > src/sim/pseudo_inst.cc 57c340f947c719a5acc3867037d51829c3967671 > util/multi/Makefile 57c340f947c719a5acc3867037d51829c3967671 > util/multi/tcp_server.hh 57c340f947c719a5acc3867037d51829c3967671 > util/multi/tcp_server.cc 57c340f947c719a5acc3867037d51829c3967671 > > Diff: http://reviews.gem5.org/r/3228/diff/ > > > Testing > ------- > > > Thanks, > > Curtis Dunham > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
