Let me tell you a story
Recently, I have been assigned as a reviewer for a patch provided by our partner and this is okay to me because as a peer of Settings app, it’s part of my duty to review these patches. But … sadly, there are so many problems in that patch and I thought “Ok, it’s fine, because we all would make mistakes at first especially if you are not familiar with the part of codes”, so I tried my best to comment down all problematic codes with references (like to MDN or some related codes) to make sure they can get familiar with them more quickly and easily.
After several rounds of this process, we finally made it and I thought this patch was good enough to be landed, so I gave them r+ on this patch.
If the story ended here, then I don’t have to write any bits of words like this article. As you may know, this is not the end.
What happened next
After signing-off this patch, there are still some other reviewing processes undergoing for different part of codes. For me, I have done my works, so I just left them alone and kept focusing on new features. Someday in the morning, I randomly checked the messages on Github and noticed there was one comment about the r+ patch, just out of curiosity, I decided to click it and checked it out.
That was a comment about missing change on the entry point and this definitely broke Settings app (You can’t even do anything). But … I was sure that this did work when I gave r+ because I did try the entry point to jump to that specific app when reviewing. So what’s going on right now !?
After a while, I finally realized that they just changed the code without any further notification and because the code change is kinda huge, no one would realize this problem when reviewing patch. So what does this mean ? If you really want to set someone up in this review process, you just have to get his/her r+ on your patch and use some magical git commands to break it and NO ONE WOULD NOTICE THIS AND WOULD BLAME ON THE REVIEWER !! (I wasn’t blamed yet, but if this patch got merged and I didn’t notice this, I would be the one)
Final
We all know this is the critical flaw in our review process, but in order to trust anyone, we haven’t forced people to tell us if the patch got changed after r+. By doing so, this would make the review process more easy and we don’t have to get stuck in some kinds of principles. Sadly, this happened to me few days ago.
Not sure what to fix here because if you are familiar with git, you can do whatever you want on commits. It is a double-edged sword which provides you so many advantages and also some shortages. I just hope this was not made on purpose …
SUCK.
