On Sat, Aug 27, 2016 at 9:00 AM, Eric Rescorla <e...@rtfm.com> wrote:
> > On Sat, Aug 27, 2016 at 8:53 AM, Gregory Szorc <gsz...@mozilla.com> wrote: > >> >> >> > On Aug 27, 2016, at 07:09, Kan-Ru Chen <kc...@mozilla.com> wrote: >> > >> >> On Sat, Aug 27, 2016, at 11:35 AM, Gregory Szorc wrote: >> >>> On Fri, Aug 26, 2016 at 8:27 PM, Steve Fink <sf...@mozilla.com> >> wrote: >> >>> >> >>>> On 08/26/2016 08:16 PM, Gregory Szorc wrote: >> >>>> >> >>>> >> >>>>> On Aug 26, 2016, at 19:54, Kan-Ru Chen <kc...@mozilla.com> wrote: >> >>>>> >> >>>>> Hello, >> >>>>> >> >>>>> In Bug 1297276 I landed a patch to rename mozilla/unused.h to >> >>>>> mozilla/Unused.h to make it more consistent with our other MFBT >> headers. >> >>>>> Normally rename a header shouldn't cause too much trouble, however >> this >> >>>>> rename is only changing the case so you might experience some >> problems >> >>>>> on case insensitive filesystem. >> >>>>> >> >>>>> As pointed out by Tim in >> >>>>> https://bugzilla.mozilla.org/show_bug.cgi?id=1297276#c19 if you use >> >>>>> |git pull -f| to update local copy of gecko and git refuse to, you >> can >> >>>>> rm mfbt/unused.* first to make git happy. >> >>>> Case only renames cause a lot of havoc. Somewhere there is an open >> bug to >> >>>> implement a server side hook to reject them. >> >>>> >> >>>> What I'm trying to say is thank you for reminding me to implement the >> >>>> hook. And congratulations on likely being the last person to perform >> a case >> >>>> only rename on the repo. >> >>> >> >>> For the record, is there a better way to accomplish this? In this >> >>> particular case, it seems like we really do want the rename. Would it >> work >> >>> better to do two commits, one from mozilla/unused.h -> >> >>> mozilla/LucyTheDancingFerret.h, then another doing >> >>> mozilla/LucyTheDancingFerret.h -> mozilla/Unused.h? >> >> >> >> >> >> No. Because if you perform an update/checkout across the rename, you >> may >> >> encounter problems. This can happen when bisecting, for example. >> >> >> >> Now, this isn't as bad as a case folding collision where you have both >> >> unused.h and Unused.h: some filesystems just flat out refuse that. >> >> >> >> At least with a rename your VCS has the opportunity to delete the old >> >> file >> >> before creating the different cased one. But even then, build systems >> and >> >> other tools can be confused because some filesystems are case >> insensitive >> >> but case preserving and tools may make inappropriate decisions about >> >> whether "unused.h" and "Unused.h" are the same file. There's a lot that >> >> can >> >> go wrong. So the whole scenario is best avoided. >> > >> > So given the fact people can make mistakes, what would you suggest if >> > one really wants to fix the filename? I hope the hook will display such >> > suggestions instead of just say "don't do this." >> >> The hook will likely link to a web page explaining in more detail what to >> do. >> >> While there is always an option to override hooks, sadly in this case I >> don't think we'll allow it too often. This also means - rather >> unfortunately - that if someone makes a casing mistake it will forever be >> enshrined in the repo. > > > I suppose we can deal with these issues as they come up, but I don't think > it's at all obvious that build/version control issues required to make the > transition work outweigh the engineering ergonomics issues of having > consistency in the code, so consider this a note that there's not really > consensus that leaving these mistakes enshrined forever is good policy. > The policy will almost certainly be that we'll need to examine case conflicts on a case-by-case basis. The server-side commit hook and linting tasks will serve as a forcing function. This doesn't come up too often, so hopefully it won't be a big deal. And, the policy will likely reflect different severity of case conflict problems. For example, renaming just a file is the least severe. However, renaming a directory or having simultaneous mixed case are much more severe. > > > > > >> The best way to avoid this is linting tests and code review. With the >> autoland repo, we'll soon drop bad commits instead of backing them out. So >> if something in automation finds case problems, the commit effectively >> never existed and there won't be a problem (beyond automation). >> >> One could whip up a filename auditing TaskCluster task pretty easily. >> Essentially copy https://hg.mozilla.org/mozilla-central/rev/f5e13a9a2e36. >> I'll happily review it. >> >> Also, I'm sorry you had to fall into this mostly unmarked trap. Others >> have likely done similar things unknowingly. You did the right thing and >> sent an email to notify people. That also happened to remind me that we >> need to fix things :) Please don't feel bad about what you did: it wasn't >> your fault. Blame the lack of tools that didn't catch this. And blame me >> for having a bug (assigned to me even!) to implement one of those tools. >> >> > >> >> The bug tracking the server hook is 797962 btw. >> _______________________________________________ >> dev-platform mailing list >> dev-platform@lists.mozilla.org >> https://lists.mozilla.org/listinfo/dev-platform >> > > _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
