Local commit hooks are even better, thanks! Are there instructions somewhere on how to set up the hooks and make sure they run properly?
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 _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
