Evidently we disagree. IMHO the communication and openness we achieved in that bug leaves a lot to be desired.
> 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. I think it was absolutely clear I was asking for an explanation for why we were doing this work. I only mentioned the breakage the patch caused to highlight that we shouldn't just write code just for the heck of it. I think this is a common confusion as well. Lots of software developers think (mostly implicitly) that their job is to make as many structured modifications to document collections as quickly as possible. Perhaps contrarily, I think we'd be better off if we only solved problems with code as a last ditch effort. *Invariably*, your code will be broken and unmaintainable, so it is (IMHO) much better from an organizational perspective that you only write code when no other good, scalable alternative exists. My first question was "Why do we need this anyway?". My second and third questions were "Is there a reason why we're doing this?" and "What is the 'user story'?". Then after asking my question three times, you answered that the bug was meant to prevent the situations where "our codebase would be unhealthy and may cause some trouble." Implicitly, you were asking me to take a few axioms on faith. Namely that 1. Engineers will, in the future, make unintentional changes to git file permissions. 2. Those unintentional changes will hurt us. I thought that those were non-trivial principles to take on faith about engineering at Mozilla, so I wanted for you to prove that they followed from a more obvious set of axioms. This is why I asked you to clarify and asked "What would be unhealthy about it?", "What trouble would it cause?", and "Has this already happened?". And, for the record, I am not a sheriff. I don't believe in sheriffing as a human responsibility. I don't even believe in release engineering as an activity that Mozilla should pursue. I'm just a gaia developer who has (for about a year now) been worried about the rate at which we're generating poorly organized, poorly tested, unmaintainable code. On Wed, Mar 26, 2014 at 10:59 AM, Greg Weng <[email protected]> wrote: > 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
