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
