On Mon, Dec 17, 2018 at 9:33 AM Kartikaya Gupta <kgu...@mozilla.com> wrote:
> Local commit hooks are even better, thanks! Are there instructions > somewhere on how to set up the hooks and make sure they run properly? > We've yet to document these hooks but enabling them is very similar to enabling the lint hooks which are documented here: < https://firefox-source-docs.mozilla.org/tools/lint/usage.html#using-a-vcs-hook>. The only difference is that you should use hooks_clang_format.py as the file name. > On Mon, Dec 17, 2018 at 9:22 AM Ehsan Akhgari <ehsan.akhg...@gmail.com> > wrote: > > > > On Fri, Dec 14, 2018 at 7:20 AM Kartikaya Gupta <kgu...@mozilla.com> > wrote: > >> > >> Personally I would prefer if we rewrote the commits locally to be > >> formatted prior to submitting it into Phabricator. That way everything > >> stays in sync. Also usually clang-formatting a patch is the last thing > >> I want to do before submission anyway. And doing it now is kind of > >> annoying if you have a multi-patch stack because by default it only > >> operates on uncommitted changes so formatting a patch that's not the > >> topmost means doing some patch juggling/rebasing. > > > > > > Right. I think that is the thinking behind the plan of enabling local > commit hooks (provided in bug 1507007) if I understand your point > correctly. That will ensure that anything that is committed will be > formatted properly, and as such it should ensure that code will be > formatted before being submitted for review, as well as making sure that > scenarios such as rebasing will also not mess up the formatting of the > commits in your local queue. > > > >> > >> > >> On Thu, Dec 13, 2018 at 5:54 PM Ehsan Akhgari <ehsan.akhg...@gmail.com> > wrote: > >> > > >> > Previously I had shied away from ideas similar to (a) or (b) since I > wasn't sure if that would be what developers would expect and accept, given > that it would cause the change the reviewer sees to no longer match their > local change, which could cause hicups e.g. when trying to find the code > that a reviewer has commented on locally, or when rebasing on top of a > newer version of mozilla-central after Lando has landed your patch (which > may cause conflicts with your local patch due to formatting differences). > >> > > >> > Do we think these types of issues aren't something we should be > concerned about? > >> > > >> > Thanks, > >> > Ehsan > >> > > >> > On Fri, Dec 7, 2018 at 3:09 PM Andrew Halberstadt <a...@mozilla.com> > wrote: > >> >> > >> >> I think we should implement a) and do the formatting prior to > submission. This prevents us from wasting reviewer time on format issues, > and also makes sure that "what you see in phab, is what gets landed". > >> >> > >> >> On Fri, Dec 7, 2018, 2:04 PM Gregory Szorc, <g...@mozilla.com> wrote: > >> >>> > >> >>> On Fri, Dec 7, 2018 at 1:57 PM Botond Ballo <bba...@mozilla.com> > wrote: > >> >>> > >> >>> > On Fri, Dec 7, 2018 at 11:36 AM Sylvestre Ledru < > sle...@mozilla.com> > >> >>> > wrote: > >> >>> > > In the meantime, we will be running a bot weekly to reformat the > >> >>> > > mistakes and add the changeset into the ignore lists. > >> >>> > > But in the long run this won’t be sustainable, so once we gain > >> >>> > > confidence that a good number of developers have successfully > integrated > >> >>> > > clang-format into their local workflow, we will look into > enabling a > >> >>> > > Mercurial hook on hg.mozilla.org to reject misformatted code > upon push > >> >>> > > time. That will be the ultimate solution to help ensure that > our code > >> >>> > > will be properly formatted at all times. > >> >>> > > >> >>> > Have you considered something like running clang-format > automatically > >> >>> > during landing (i.e. as part of what Lando does to the patch)? > That > >> >>> > seems like it would achieve the best of both worlds, by not > placing > >> >>> > any requirements on what developers do locally, while also > ensuring > >> >>> > the tree is always properly formatted without cleanup commits. > >> >>> > > >> >>> > >> >>> I chatted with Sylvestre earlier today. While I don't want to speak > for > >> >>> him, I believe we both generally agree that the formatting should > happen > >> >>> "automagically" as part of the patch review and landing lifecycle, > even if > >> >>> the client doesn't have their machine configured for formatting on > save. > >> >>> This would mean that patches are either: > >> >>> > >> >>> a) auto-formatted on clients as part of being submitted to > Phabricator > >> >>> b) updated automatically by bots after submission to Phabricator > >> >>> c) auto-formatted by Lando as part of landing > >> >>> > >> >>> Lando rewrites/rebases commits as part of landing, so commit hashes > already > >> >>> change. So if auto-formatting magically occurs and "just works" as > part of > >> >>> the review/landing process, there should be little to no developer > >> >>> inconvenience compared to what happens today. i.e. developers don't > need to > >> >>> do anything special to have their patches land with proper > formatting. > >> >>> _______________________________________________ > >> >>> dev-platform mailing list > >> >>> dev-platform@lists.mozilla.org > >> >>> https://lists.mozilla.org/listinfo/dev-platform > >> > > >> > > >> > > >> > -- > >> > Ehsan > > > > > > > > -- > > Ehsan > -- Ehsan _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
