Those are all good points. I would suggest putting the commit explanations into the code instead of commit messages, keeping the commit messages empty. When we have a good commit message, we can just copy it into the code as a comment.
It's understandable that people don't understand some of my patches. I also don't understand some of other people's patches. It's normal. I don't think commit messages always help. If you don't work on a specific subtree on a regular basis, it's normal that over time you'll stop understanding it. Marek On Tue, Nov 20, 2018 at 5:28 AM Timothy Arceri <tarc...@itsqueeze.com> wrote: > On 20/11/18 5:46 pm, Marek Olšák wrote: > > On Tue, Nov 20, 2018 at 12:08 AM Dave Airlie <airl...@gmail.com > > <mailto:airl...@gmail.com>> wrote: > > > > On Tue, 20 Nov 2018 at 14:42, Marek Olšák <mar...@gmail.com > > <mailto:mar...@gmail.com>> wrote: > > > > > > On Mon, Nov 19, 2018 at 7:15 PM Bas Nieuwenhuizen > > <b...@basnieuwenhuizen.nl <mailto:b...@basnieuwenhuizen.nl>> wrote: > > >> > > >> So I tried to test this with radv and got a bunch of crashes in > CTS, > > >> mostly around 3d image support: > > >> > > >> #3 0x00007ffff71a9396 in __assert_fail () from > /usr/lib/libc.so.6 > > >> #4 0x00007ffff69da3b4 in > > >> Addr::V2::Gfx9Lib::HwlGetPreferredSurfaceSetting > > (this=0x555557661b30, > > >> pIn=0x7fffffffd5f0, pOut=0x7fffffffd5d0) > > >> at ../mesa/src/amd/addrlib/src/gfx9/gfx9addrlib.cpp:3684 > > >> #5 0x00007ffff69cf331 in > > >> Addr::V2::Lib::Addr2GetPreferredSurfaceSetting > (this=0x555557661b30, > > >> pIn=0x7fffffffd5f0, pOut=0x7fffffffd5d0) > > >> at ../mesa/src/amd/addrlib/src/core/addrlib2.cpp:1742 > > >> #6 0x00007ffff69c4e87 in Addr2GetPreferredSurfaceSetting > > >> (hLib=0x555557661b30, pIn=0x7fffffffd5f0, pOut=0x7fffffffd5d0) > > >> at ../mesa/src/amd/addrlib/src/addrinterface.cpp:1697 > > >> #7 0x00007ffff69bf8d4 in gfx9_get_preferred_swizzle_mode > > >> (addrlib=0x555557661b30, in=0x7fffffffd690, is_fmask=false, > > >> flags=33555202, swizzle_mode=0x7fffffffd698) > > >> > > >> It seems to be caused by the explicit swizzle mode override that > > we do with > > >> > > >> commit b64b7125586ce48232658cd860f549a6139b6ddd > > >> Author: Marek Olšák <marek.ol...@amd.com > > <mailto:marek.ol...@amd.com>> > > >> Date: Mon Apr 2 12:54:52 2018 -0400 > > >> > > >> ac/surface/gfx9: request desired micro tile mode explicitly > > >> > > >> Tested-by: Dieter Nützel <die...@nuetzel-hh.de > > <mailto:die...@nuetzel-hh.de>> > > >> > > >> > > >> Since we never got a reason to have it (the commit message above > is > > >> not descriptive and the patch not reviewed) and this is the > second > > >> time already that this breaks stuff (The other was allowing S > tiling > > >> for raven displayable surfaces, per 7eff8d7d3564), maybe revert > > it and > > >> let addrlib make the decision? > > > > > > > > > Yes, my commits are mostly unreviewed. It's the norm now. Willing > > reviewers don't exist anymore. I don't really mind that my patches > > are not reviewed, but whoever complains that I push unreviewed > > commits should ask himself why he didn't review them in their review > > period. That applies to everybody. Either review regularly or accept > > that unreviewed commits are normal. > > > > > > Secondly, past commits can't break future commits, so don't say > > it breaks stuff again. It's illogical. > > > > > > There may be multiple reasons why the commit exists. As long as > > reverting it doesn't break piglit / radeonsi, I'm OK with the > reverting. > > > > Marek, > > > > There is no way anybody could review this commit, the commit log > > contains 0 information on why or what the commit is doing or what it > > fixes, there is nothing to say what the reviewer is looking out for. > > > > > > It's something you could have mentioned on the review though. Instead > > you decided to be silent. I'm not blaming you. I understand that you > > probably had no time to review radv-related patches at that time. There > > however has to be some reviewer if you care about the long-term outcome > > whether you personally have time or not. 0 information is not an excuse > > not to review. Had there been any explanation in the commit, it wouldn't > > have made any difference on the current situation, other than perhaps > > preventing this pointless discussion that doesn't help anybody. > > > > Marek > > As per the reply from Bas. Please take this as feedback, and not an > attack. But the reason I don't even attempt to review most of your > radeonsi patches is because the commit messages are empty. Reviews are > are two way street, the time you save not writing a commit message just > cost the reviewer time (probably more) in trying to comprehend what is > going on. Double that time again (at least) for someone who doesn't know > the driver intimately. > > As far as asking for a commit messages as part of review, I have done > this on a number of occasions but it shouldn't really be on the reviewer > to do this. In pretty much any other driver not having a proper commit > message is pretty much an instant no go for anything other than truly > self explanatory commits. > > Besides assisting review, I find commit messages an invaluable learning > tool. As well as learning by just browsing/reviewing patch submissions > the power of git unlocks these invaluable resources in an instant. Don't > know what x code does or confused why it does something unexpected then > a quick git log -S"string of code" will usually answer the question > pretty quickly. Unfortunately I've done just this a number of times in > radeonsi only to be greeted by an empty commit message. Again the same > advantages of having a good commit message are seen when bisecting, etc. > > As far as staying silent on this feedback I have considered saying > something about this for sometime. However I've avoided it so far as to > avoid causing offense, or come across as lecturing a fellow open source > dev on what seems like common knowledge. > > Now whether better commit messages would mean I would review more code > is hard to say because I still don't know the driver that well. But it > would make me feel more comfortable giving an ack to at least say I've > read over the code and everything seems to make sense. I tend to look > over most of you series currently but don't make any comments because I > have no idea why a lot of the changes were made. > > Before I go I'll just say writing good commit messages is something we > all need to work on, it can be easy to slip into habits of not doing so. > I'm more than guilty of writing my fair share of less than ideal > messages but I think in the long run the effort spent getting these > things right is well worth it. >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
