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 <[email protected]> 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 <[email protected]> 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, <[email protected]> wrote: >>> >>> On Fri, Dec 7, 2018 at 1:57 PM Botond Ballo <[email protected]> wrote: >>> >>> > On Fri, Dec 7, 2018 at 11:36 AM Sylvestre Ledru <[email protected]> >>> > 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 >>> [email protected] >>> https://lists.mozilla.org/listinfo/dev-platform > > > > -- > Ehsan _______________________________________________ dev-platform mailing list [email protected] https://lists.mozilla.org/listinfo/dev-platform
