I don't think what we discussed are totally about what you mentioned. Although I definitely agree what you want mentioned about communication and questions you've listed.
One thing made me totally didn't understand is that according to your own comments, you *did* repeatedly complain one error that caused the patch broken, even after the patch got updated. Unless Bugzilla cheated me, the text I've read was not you've written. And according to your suggestions, Bug 970138: - Is this a good idea? > Yes, it's a good idea if we can implement it correctly. On the contrary, to > allow people change file permissions arbitrarily is the really bad idea. - What problem does it solve (and for whom)? > The permission problem which had happened (see comments #24), and may happen > again in the future. - Is it high priority? > No. But can we say we now only handle high priority bugs? - Are there any risks involved in doing this? > If it get implemented correctly, I don't see any risks but the whitelist must > be maintained by people. - Who should I ask for input? > Input? Don't know what exactly you mean... - Who should at least know that this is happening? > Gareth Aye, as a sheriff > Yuren Ju, as a build system peer > Tim Guan-tin Chien, as the module owner of the Gaia' --- Then here is the history according to the comments: #16 Gareth Aye [:gaye] Backed out https://github.com/mozilla-b2g/gaia/commit/6c93da506e79bd7641f2cc0958b531ab84ceefe1 because 644 doesn't work for travis scripts in ./tests/travis_ci/. Why do we need this anyway? #17 Tzu-Wen Lin [:waynux] Shall we have different file permissions in the case? Thanks for comments. #18 Yuren [:yurenju][:小朱] Tzu-Wen, we should only apply this hook for a whitelist include apps, dogfood_apps, test_apps, test_external_apps, and please file a follow up bug to fix all permission for those directories. --- So here are what you said about the first time backout. And we realize the error and it got corrected at: #20 Tzu-Wen Lin [:waynux] The patch is updated. The script now only check out the whitelist folders. Travis is green. Thanks for helps, Yuren. #21 Gareth Aye [:gaye] Is there a reason why we're doing this? What is the "user story"? A patch for this broke us before, so there are cons here... --- Yes, here you asked the user store again. But your conclusion "A patch for this broke us before, so there are cons here..." seems that completely complained the patch due to the error. What I've read is "This patch broken somthing" --> "It must be wrong" Or, maybe I misunderstood what the "Foo do something, so there are bar" statement means. #22 Greg Weng [:snowmantw][:gweng][:λ] > The broken part should be fixed in the waynux's newest patch. I don't think > the cons are because of the fundamental flaw of this patch, but the implementation that we can fix. Here I answered according to your complain "A patch for this broke us before". As you said, the patch broke us *before*, and I told you it's an error would be corrected in the updated patch. > As you may know, git would commit files include permission changes. So, if > you change the file to some improper permission and then commit, for example, 000 (if possible), our codebase would be unhealthy and may cause some trouble. At the commenting time, I didn't know what's wrong to prevent an error commit. > And the broken parts are even able to be fixed via the patch: if someone > change some travis scripts' permissonis in ./tests/travis_ci/, this patch (if it was implemented correctly) can stop it. The patch can even prevent what it caused broken. --- #23 Gareth Aye [:gaye] > What would be unhealthy about it? What trouble would it cause? Has this > already happened? We have enough bugs in our software that (in the case that we haven't already experienced an issue) we do not need to be creatively discovering infrastructural issues. > This bug has caused trouble and no one has made a defensible case for it > solving any *reported* issues. You repeated what you had complained about: "A patch for this broke us before, so there are cons here..." So, in your opinion, the bug had "caused trouble" in the past time *and* there lacks motivations to patch it. #24 Greg Weng [:snowmantw][:gweng][:λ] > I don't know why you just take one implementation error as it's a fundamental > flaw of this patch (and bug). > The problem is not caused by this bug *but an immature patch*, which is > unfortunately need to be backed out and fixed as other REOPENED patches. The patch had broken something, and it desired a back-out. This is correct, and there is no excuse. But I really don't know that why you complain the past error again, even when it already got corrected in the updated version. > If you disagree with the permission check, you may file a better way to > prevent people change permissions in our codebase arbitrarily and would not > put extra effort on reviewers' eyes. It seems you don't like the solution and even don't like the problem. So I asked if you have better idea to make this thing done. > As far as I know, at least one reviewer has reviewed some problematic commits > that changed permissions incorrectly although the commiter was unintentional > (caused by the Windows file system), > and this cost him extra time to guide the commiter how to fix this on the > commiter's local machine. Another real case is that we've been asked that why > some files (JavaScript files) in our codebase > are not 0644 but 0755, although it does no harm to keep developing. This answered your "what" questions. If you're not satisfied the answer, you can reply on Bugzilla. But I don't even know if you ever read these, because you reply nothing. There's neither supporting or opposing. > And I'm curious that you seems even didn't comment on the patch to see what > he has updated if you concerns the *legacy* error, which should be fixed now. > If they're still existing, > you of course can list them and block the new patch as what every reviewer > and peers would do. However, according to your comments, you seems just > focused on the error existed in the previous version. > I don't know why we should review or feedback on a patch focusing on the > passed version, but not the updated version. Accused what? "A patch for this broke us before, so there are cons here..." "This bug has caused trouble and no one has made a defensible case for it solving any *reported* issues." Which comment was fabricated? > And I must say that I defend the patch and the bug *NOT* because the reporter > is me, but the above problems. In fact, this idea and the problems was > discussed among some people, and I were unfortunately the reporter (the most > junior one, who may not know so much about feedbacking on a backed out > patch), and seems the only one person replied your questions so far. I don't know why only me left to do these arguing things. Yes, I can just say "Oh, okay, it's my fault. Please close this bug" and let it pass, but it would solve nothing. 2014-03-26 21:09 GMT+08:00 Gareth Aye <[email protected]>: > Hi Gaia, > > tl;dr Please make a habit of explaining what the high-level goals of a bug > are when you file one. If you do one thing well as a software developer, > please make it communication. > > /* The longer version */ > > Sometimes our software doesn't work like we would like for it to. When this > happens at Mozilla, it's protocol to file a bug. Why is that? We file bugs > because we're open and being open means being transparent about the work > we're doing and why we're doing it. Often (and regrettably), I see bugs > filed like this one. > > In bug 970138, Greg suggests that we should add a git pre-commit hook to > prevent patch authors from changing the file permissions on code that gets > checked into gaia. This is not commentary on whether or not that should be > done. This is commentary on how some folks came to the decision that it > should be done and how they communicated it with the organization and > community. > > What was the rationale for this being prioritized and fixed? Greg's and > John's initial justification is that it "should be [fixed]". Is this > justification sufficient? :waynux and :yurenju seemed to think so since they > (respectively) patched and reviewed the bug. > > Interestingly, the pre-commit hook prevented me from making changes to the > scripts in $GAIA/tests/travis_ci/. I had to go back and in the history to > see where the pre-commit hook was introduced and then back it out. Then I > asked *three times* for explicit, high-level justification for the work that > was being done. > > The first time I asked, the patch author promptly made modifications to the > pre-commit hook and the reviewer made some more code-level suggestions. > > The second time I asked, Greg told me that the "broken part" of the patch > had been fixed by the update. > > The third time I asked, I was accused of not recognizing that the problem I > reported had been fixed by modifications to the pre-commit hook. > > By this point, I am feeling like you had one job(tm) and let me be super > explicit. You are doing it all wrong! Your most important job as a software > developer is to communicate. This gets conflated by the fact that we have > lots of responsibilities at Mozilla. Amongst other things, I have to know > which css selectors are performant, write robust tests, understand the > caldav protocol, know the codenames for about 8 releases and 20 phones, and > never ever ever use localStorage. All of those things come second to > communication. > > Good communication takes several forms. Sometimes, it means writing a test > so that you can express to readers and coders (present and future) what your > software is supposed to do. Sometimes it means estimating the risk > associated with introducing a new feature into our unstable codebases. Below > I've compiled a short list of questions we should all ask ourselves when we > file, fix, and review bugs. When we answer these questions for ourselves, we > are being open when we mirror our answers to bugzilla. > > - Is this a good idea? > - What problem does it solve (and for whom)? > - Is it high priority? > - Are there any risks involved in doing this? > - Who should I ask for input? > - Who should at least know that this is happening? > > Best, > Gareth > > _______________________________________________ > dev-gaia mailing list > [email protected] > https://lists.mozilla.org/listinfo/dev-gaia > -- Greg Weng http://about.me/snowmantw Understand y f = f [ y f ] ; lose last remaining non-major friend -- Anonymous _______________________________________________ dev-b2g mailing list [email protected] https://lists.mozilla.org/listinfo/dev-b2g
