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.
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 _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
