Re: Automatic changes during the dev workflow [was: Re: Upcoming changes to our C++ Coding Style]

[Ehsan Akhgari]

On Mon, Dec 17, 2018 at 1:22 PM Steve Fink  wrote:

> In theory, I would definitely prefer if all the code were auto-formatted
> during regular development(*), but in practice the performance means
> it's not as transparent as it needs to be -- I have noticed that since
> enabling the format-source extension, rebasing my patch stacks is
> significantly slower.
>

Please if you see performance issues, file them with some steps to
reproduce and CC Andi, he may be able to help.  We should definitely not
have to avoid running our tools due to performance issues.  :-)

Cheers,
-- 
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Automatic changes during the dev workflow [was: Re: Upcoming changes to our C++ Coding Style]

[Ehsan Akhgari]

On Fri, Dec 14, 2018 at 4:55 PM Kartikaya Gupta  wrote:

> On Fri, Dec 14, 2018 at 1:58 PM Sylvestre Ledru 
> wrote:
> > We have more and more tools at review phase (clang-format, flake8,
> eslint, clang-tidy, codespell, etc) which propose some auto-fixes.
>
> Honestly I find it quite annoying when I'm trying to review a patch
> and the phabricator diff is filled up with bot comments about
> formatting. As a reviewer, I shouldn't have to think about formatting,
> and it shouldn't fill up my screen and prevent me from looking at the
> actual code.
>
> > Currently, the turn around time of the tools is 14m on average which is
> usually faster than the reviewer looking at the patch.
>
> Usually, but not always. And 14 minutes is still long enough that the
> person writing the patch has context-switched away and is in the
> middle of doing something else when the bot comes a-knocking.
>

What if we *rejected* misformatted patches in Phabricator as opposed to
just comment on them?  That is, what if the bot would mark the reviews as
"Request Changes" when it found any problems, IOW it would review the patch
alongside with normal reviewers.  As Sylvestre mentioned the diffs on
individual lines is about to go away, but this way we'd also make sure that
misformatting caught at review time would actually block the landing of the
patch.


> > If Phabricator provides the capability, we could have the bot
> automatically proposing a new patchset on top on the normal one.
> > The reviewer would look at the updated version.
>
> This would be fine for the reviewer, but...
>
> > The main drawback would be that developer would have to retrieve the
> updated patch
> > before updating it.
>
> That's a chore too. For the developer writing the patch I feel like
> there should be a way to just say "please format this patch in-place
> for me" (basically what I expected `./mach clang-format` to do, except
> it doesn't quite - see bug 1514279) and even that should be opt-in.
> IMO the default behaviour should just be to automatically apply
> formatting prior to submission, without anybody having to think about
> it except in cases where the developer needs to apply "//clang-format
> off" blocks.
>

Right.  I think the ideal state would be for everyone to use editor
integration and local VCS hooks.  The VCS hooks we can enable in ./mach
bootstrap but editor integration is something people will need to opt into
turning on.  Once we get there, then I think formatting will be mostly
taken care of transparently for you without needing to think about it
consciously.

Doing things like rejecting misformatted code at review time may
incentivize people updating their local configuration.  I'm thinking of
reasons why that may break people's workflow but can't think of any right
now, curious to know if anyone else can?

-- 
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Automatic changes during the dev workflow [was: Re: Upcoming changes to our C++ Coding Style]

[Steve Fink]

In theory, I would definitely prefer if all the code were auto-formatted 
during regular development(*), but in practice the performance means 
it's not as transparent as it needs to be -- I have noticed that since 
enabling the format-source extension, rebasing my patch stacks is 
significantly slower. That's turning out to be a problem because it 
tends to take long enough to trigger a context switch, which is 
especially problematic when attempting to land something, wandering off 
during the rebase, and then realizing that too much time has elapsed and 
I'm going to need to start over and pull again because new changes have 
come in. (Maybe this just means I should get friendly with Lando, but 
even then I tend to have quite a few rebases that I want to tend to 
manually while in the testing/debugging phase.)


I know we collect build time statistics when running `mach build` that 
we can opt into uploading; would it be possible to collect timings for 
eg `hg rebase` operations? Some fancy new gps extension? :-) You 
probably shouldn't trust my gut "it feels slower" impressions too far.


 - sfink


(*) Now that we've taken the hit of uglifying the code, it seems like we 
should get as much mileage as we can out of it.


(I still think it was the right decision to go to a single style, and it 
was better to just pick one rather than blocking on arguing it down to a 
some "globally optimal" style first.)



On 12/14/2018 10:57 AM, Sylvestre Ledru wrote:
I think we should aim at option b) (updated automatically by bots 
after submission to Phabricator)


We have more and more tools at review phase (clang-format, flake8, 
eslint, clang-tidy, codespell, etc) which propose some auto-fixes.


Currently, the turn around time of the tools is 14m on average which 
is usually faster than the reviewer looking at the patch.
If Phabricator provides the capability, we could have the bot 
automatically proposing a new patchset on top on the normal one.

The reviewer would look at the updated version.

By doing that, we would save time for everyone. The main drawback 
would be that developer would have to retrieve the updated patch

before updating it.

Sylvestre


Le 13/12/2018 à 23:53, Ehsan Akhgari a écrit :
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 > 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, mailto:g...@mozilla.com>> wrote:

    On Fri, Dec 7, 2018 at 1:57 PM Botond Ballo
    mailto:bba...@mozilla.com>> wrote:

    > On Fri, Dec 7, 2018 at 11:36 AM Sylvestre Ledru
    mailto: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 

Re: Upcoming changes to our C++ Coding Style

[Ehsan Akhgari]

On Mon, Dec 17, 2018 at 9:33 AM Kartikaya Gupta  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 
> wrote:
> >
> > On Fri, Dec 14, 2018 at 7:20 AM Kartikaya Gupta 
> 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 
> 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 
> 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,  wrote:
> >> >>>
> >> >>> On Fri, Dec 7, 2018 at 1:57 PM Botond Ballo 
> 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.
> >> >>> 

Re: Upcoming changes to our C++ Coding Style

[Kartikaya Gupta]

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  wrote:
>
> On Fri, Dec 14, 2018 at 7:20 AM Kartikaya Gupta  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  
>> 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  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,  wrote:
>> >>>
>> >>> On Fri, Dec 7, 2018 at 1:57 PM Botond Ballo  wrote:
>> >>>
>> >>> > On Fri, Dec 7, 2018 at 11:36 AM Sylvestre Ledru 
>> >>> > 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

Re: Upcoming changes to our C++ Coding Style

[Ehsan Akhgari]

On Fri, Dec 14, 2018 at 7:20 AM Kartikaya Gupta  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 
> 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 
> 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,  wrote:
> >>>
> >>> On Fri, Dec 7, 2018 at 1:57 PM Botond Ballo 
> wrote:
> >>>
> >>> > On Fri, Dec 7, 2018 at 11:36 AM Sylvestre Ledru 
> >>> > 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

Re: Automatic changes during the dev workflow [was: Re: Upcoming changes to our C++ Coding Style]

[Sylvestre Ledru]

Le 14/12/2018 à 22:55, Kartikaya Gupta a écrit :

On Fri, Dec 14, 2018 at 1:58 PM Sylvestre Ledru  wrote:

We have more and more tools at review phase (clang-format, flake8, eslint, 
clang-tidy, codespell, etc) which propose some auto-fixes.

Honestly I find it quite annoying when I'm trying to review a patch
and the phabricator diff is filled up with bot comments about
formatting. As a reviewer, I shouldn't have to think about formatting,
and it shouldn't fill up my screen and prevent me from looking at the
actual code.


We received this feedback from several folks and we agree (initially, we were 
showing the diff itself but it wasn't
bringing much).
This is fixed in staging and the fix should go in the next release of the 
platform (Dec 22nd)

Cheers

S


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Automatic changes during the dev workflow [was: Re: Upcoming changes to our C++ Coding Style]

[Jean-Yves Avenard]

Hi

> On 14 Dec 2018, at 7:57 pm, Sylvestre Ledru  wrote:
> 
> I think we should aim at option b) (updated automatically by bots after 
> submission to Phabricator)
> 
> 

I don’t particularly fancy this idea. Finding yourself with different code on 
Phabricator and locally is a good way to shoot yourself in the foot.

Preventing pushing non-properly formatted code, it’s so easy to properly format 
your code. Also make you have on extra check on what you’re about to review.

Similar to the Google review process. IIRC before anyone is asked to review 
anything, the submission must pass a set of tests, one of them includes 
checking the coding style.

> We have more and more tools at review phase (clang-format, flake8, eslint, 
> clang-tidy, codespell, etc) which propose some auto-fixes.
> 
> Currently, the turn around time of the tools is 14m on average which is 
> usually faster than the reviewer looking at the patch.
> If Phabricator provides the capability, we could have the bot automatically 
> proposing a new patchset on top on the normal one.
> The reviewer would look at the updated version.
> 
> 

It does feel longer that that :)

> By doing that, we would save time for everyone. The main drawback would be 
> that developer would have to retrieve the updated patch 
> before updating it.
> 
> 

A big drawback

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Upcoming changes to our C++ Coding Style

[Kartikaya Gupta]

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  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  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,  wrote:
>>>
>>> On Fri, Dec 7, 2018 at 1:57 PM Botond Ballo  wrote:
>>>
>>> > On Fri, Dec 7, 2018 at 11:36 AM Sylvestre Ledru 
>>> > 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

Automatic changes during the dev workflow [was: Re: Upcoming changes to our C++ Coding Style]

[Sylvestre Ledru]

I think we should aim at option b) (updated automatically by bots after 
submission to Phabricator)


We have more and more tools at review phase (clang-format, flake8, 
eslint, clang-tidy, codespell, etc) which propose some auto-fixes.


Currently, the turn around time of the tools is 14m on average which is 
usually faster than the reviewer looking at the patch.
If Phabricator provides the capability, we could have the bot 
automatically proposing a new patchset on top on the normal one.

The reviewer would look at the updated version.

By doing that, we would save time for everyone. The main drawback would 
be that developer would have to retrieve the updated patch

before updating it.

Sylvestre


Le 13/12/2018 à 23:53, Ehsan Akhgari a écrit :
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 > 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, mailto:g...@mozilla.com>> wrote:

On Fri, Dec 7, 2018 at 1:57 PM Botond Ballo
mailto:bba...@mozilla.com>> wrote:

> On Fri, Dec 7, 2018 at 11:36 AM Sylvestre Ledru
mailto: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

Re: Automatic changes during the dev workflow [was: Re: Upcoming changes to our C++ Coding Style]

[Kartikaya Gupta]

On Fri, Dec 14, 2018 at 1:58 PM Sylvestre Ledru  wrote:
> We have more and more tools at review phase (clang-format, flake8, eslint, 
> clang-tidy, codespell, etc) which propose some auto-fixes.

Honestly I find it quite annoying when I'm trying to review a patch
and the phabricator diff is filled up with bot comments about
formatting. As a reviewer, I shouldn't have to think about formatting,
and it shouldn't fill up my screen and prevent me from looking at the
actual code.

> Currently, the turn around time of the tools is 14m on average which is 
> usually faster than the reviewer looking at the patch.

Usually, but not always. And 14 minutes is still long enough that the
person writing the patch has context-switched away and is in the
middle of doing something else when the bot comes a-knocking.

> If Phabricator provides the capability, we could have the bot automatically 
> proposing a new patchset on top on the normal one.
> The reviewer would look at the updated version.

This would be fine for the reviewer, but...

> The main drawback would be that developer would have to retrieve the updated 
> patch
> before updating it.

That's a chore too. For the developer writing the patch I feel like
there should be a way to just say "please format this patch in-place
for me" (basically what I expected `./mach clang-format` to do, except
it doesn't quite - see bug 1514279) and even that should be opt-in.
IMO the default behaviour should just be to automatically apply
formatting prior to submission, without anybody having to think about
it except in cases where the developer needs to apply "//clang-format
off" blocks.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Upcoming changes to our C++ Coding Style

[Ehsan Akhgari]

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  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,  wrote:
>
>> On Fri, Dec 7, 2018 at 1:57 PM Botond Ballo  wrote:
>>
>> > On Fri, Dec 7, 2018 at 11:36 AM Sylvestre Ledru 
>> > 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

Re: Upcoming changes to our C++ Coding Style

[Mike Hommey]

On Fri, Dec 07, 2018 at 03:08:51PM -0500, Andrew Halberstadt 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".

Also, that would make it clear if weird formatting would entirely
be due to clang-format (in which case, we should probably file bugs),
and not a person's doing.

Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Upcoming changes to our C++ Coding Style

[Andrew Halberstadt]

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,  wrote:

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

Re: Upcoming changes to our C++ Coding Style

[Gregory Szorc]

On Fri, Dec 7, 2018 at 1:57 PM Botond Ballo  wrote:

> On Fri, Dec 7, 2018 at 11:36 AM Sylvestre Ledru 
> 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

Re: Upcoming changes to our C++ Coding Style

[Botond Ballo]

On Fri, Dec 7, 2018 at 11:36 AM Sylvestre Ledru  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.

Cheers,
Botond
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Upcoming changes to our C++ Coding Style

[Sylvestre Ledru]

Le 05/12/2018 à 00:22, Botond Ballo a écrit :
> Hi, I'd like to ask a couple of clarifications about process here: * 
Is clang-format invoked automatically as part of a pre-commit hook, or 
something like that?


We have been working on that. There is a pending patch 
https://bugzilla.mozilla.org/show_bug.cgi?id=1507007

We will enable it as part of the ./mach bootstrap.
> * If not, should we be invoking it manually before submitting a patch?

Yes, I think this should be done as part of the developer workflows 
now.  Each developer’s local development environment is different but 
we’re hoping that for many people this actually shouldn’t mean running 
./mach clang-format manually.  The recommended setup is to integrate 
clang-format with your code editor using one of the integrations 
documented here: 
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style/Formatting_C++_Code_With_clang-format#Editor_Integrations


If you have tips to share about editor integrations, we’d appreciate 
keeping that page updated!


And note that if you submit patches to Phabicator for review, a review 
bot will automatically check your patches for formatting mistakes, and 
if it detects them will provide a link to a patch that can be applied 
locally to fix the issues.



> * If we submit a patch without having invoked it, will violations be 
caught in automation? Will they trigger a backout?


We have been discussing about that. We don’t think we should backout a 
C++ patch because of a missing space or line return and for now we 
should focus on helping more developers update their local setup to run 
clang-format as part of their local development workflow. Developing 
local commit hooks, asking people to enable editor integrations and the 
Phabricator review bot are efforts in this category.
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.


Ehsan & Sylvestre
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Upcoming changes to our C++ Coding Style

[Botond Ballo]

Hi,

I'd like to ask a couple of clarifications about process here:

* Is clang-format invoked automatically as part of a pre-commit hook,
or something like that?
* If not, should we be invoking it manually before submitting a patch?
* If we submit a patch without having invoked it, will violations be
caught in automation? Will they trigger a backout?

Thanks,
Botond
On Thu, Nov 22, 2018 at 12:36 PM Felipe G  wrote:
>
>  On Thu, Nov 22, 2018 at 3:07 PM Ehsan Akhgari 
> wrote:
>
> > Felipe, gps and I talked a bit about adding a similar
> > .hg-blame-ignore-revs file in the tree which can contain Mercurial sha1
> > changeset IDs for the rewrite commits to be skipped over, and people would
> > be able to use the file through an alias that can be configured in ~/.hgrc
> > (possible to set it up via ./mach bootstrap).  Not sure if Felipe's
> > investigations have lead to results yet.
> >
>
> Indeed they have. I'm working on this in two separate bugs:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1508002 is implementing the
> same functionality in mercurial from`git hyper-blame`, and
> https://bugzilla.mozilla.org/show_bug.cgi?id=1508324 will be used to
> generate the list of past changesets
>
> Hope to have these wrapped up soon, and I'll write about them once they're
> done. One thing to mention in advance is that gps asked to not use the word
> "blame" as the Mercurial project doesn't like the negative connotations
> that it brings, so I'm leaning towards `hg smart-annotate` and "#
> ignore-changeset" for the string to be included in the commit message. Let
> me know if anyone has thoughts about these (or leave comments in the bug)
>
> Felipe
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Upcoming changes to our C++ Coding Style

[Felipe G]

 On Thu, Nov 22, 2018 at 3:07 PM Ehsan Akhgari 
wrote:

> Felipe, gps and I talked a bit about adding a similar
> .hg-blame-ignore-revs file in the tree which can contain Mercurial sha1
> changeset IDs for the rewrite commits to be skipped over, and people would
> be able to use the file through an alias that can be configured in ~/.hgrc
> (possible to set it up via ./mach bootstrap).  Not sure if Felipe's
> investigations have lead to results yet.
>

Indeed they have. I'm working on this in two separate bugs:

https://bugzilla.mozilla.org/show_bug.cgi?id=1508002 is implementing the
same functionality in mercurial from`git hyper-blame`, and
https://bugzilla.mozilla.org/show_bug.cgi?id=1508324 will be used to
generate the list of past changesets

Hope to have these wrapped up soon, and I'll write about them once they're
done. One thing to mention in advance is that gps asked to not use the word
"blame" as the Mercurial project doesn't like the negative connotations
that it brings, so I'm leaning towards `hg smart-annotate` and "#
ignore-changeset" for the string to be included in the commit message. Let
me know if anyone has thoughts about these (or leave comments in the bug)

Felipe
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Upcoming changes to our C++ Coding Style

[Ehsan Akhgari]

On Thu, Nov 22, 2018 at 4:07 AM Jean-Yves Avenard 
wrote:

> Hi
>
> > On 21 Nov 2018, at 3:54 am, Ehsan Akhgari 
> wrote:
> >
> >
> > You will break the blame on VCS with this change
> >
> > Yes and no. Of course, just like many tree-wide mass changes in the past
> > (e.g. the MPL2 header update), this will remain in the log.
> >
> > Mercurial and Git both support a -w argument to ignore whitespace with
> > annotate/blame.
> >
> > In addition, modern versions of Mercurial have `hg annotate --skip
> > ` which allows you to specify a revset used to select revisions
> to
> > skip over when annotating.
> >
> > Last but not least, we will tag the changeset’s commit message with
> > “skip-blame” so that Mercurial would automatically ignore the reformat
> > changeset for blame operations.
>
> I’ve found the Google’s depot_tools hyper-blame particularly useful here.
>
> It takes a .git-blame-ignore-revs file containing the list of commits to
> ignore.
>
> $ cat .git-blame-ignore-revs
> abd6d77c618998827e5ffc3dab12f1a34d6ed03d
>
> That’s with Sylvestre single commit changing dom/media (hg SHA1:
> 0ceae9db9ec0be18daa1a279511ad305723185d4)
>
> $ git clone
> https://chromium.googlesource.com/chromium/tools/depot_tools.git
> $ export PATH=$PATH:$PWD/depot_tools
>
> now git hyper-blame will behave in the same fashion as git blame, but
> ignore that particular commit.
>

Indeed.  Kats was asking me about how we can possibly support skipping over
these commits in searchfox.org, and I pointed this tool to him as well.
I'm hoping that he would be able to find a way to integrate hyper-blame
with searchfox so that more people can benefit from this.  (I am not sure
how frequently people run blame tools locally...)

For Mercurial, "# skip-blame" is just a text token that Mercurial's revset
syntax can parse out of commit messages.  The revset syntax also supports
reading external files.  Felipe, gps and I talked a bit about adding a
similar .hg-blame-ignore-revs file in the tree which can contain Mercurial
sha1 changeset IDs for the rewrite commits to be skipped over, and people
would be able to use the file through an alias that can be configured in
~/.hgrc (possible to set it up via ./mach bootstrap).  Not sure if Felipe's
investigations have lead to results yet.


> I’m guessing we could make this .git-blame-ignore-revs part of the tree,
> assuming though everyone must use git-cinnabar.
>

Indeed we can.  I would be happy to review a patch.  I think the file can
contain both the cinnabar and the gecko-dev sha1 variants of the rewrite
commit(s), since sha1s that are non-existent in a repo will just be ignored.

Cheers,
-- 
Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Upcoming changes to our C++ Coding Style

[Jean-Yves Avenard]

Hi

> On 21 Nov 2018, at 3:54 am, Ehsan Akhgari  wrote:
> 
> 
> You will break the blame on VCS with this change
> 
> Yes and no. Of course, just like many tree-wide mass changes in the past
> (e.g. the MPL2 header update), this will remain in the log.
> 
> Mercurial and Git both support a -w argument to ignore whitespace with
> annotate/blame.
> 
> In addition, modern versions of Mercurial have `hg annotate --skip
> ` which allows you to specify a revset used to select revisions to
> skip over when annotating.
> 
> Last but not least, we will tag the changeset’s commit message with
> “skip-blame” so that Mercurial would automatically ignore the reformat
> changeset for blame operations.

I’ve found the Google’s depot_tools hyper-blame particularly useful here.

It takes a .git-blame-ignore-revs file containing the list of commits to ignore.

$ cat .git-blame-ignore-revs 
abd6d77c618998827e5ffc3dab12f1a34d6ed03d

That’s with Sylvestre single commit changing dom/media (hg SHA1: 
0ceae9db9ec0be18daa1a279511ad305723185d4)

$ git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git
$ export PATH=$PATH:$PWD/depot_tools

now git hyper-blame will behave in the same fashion as git blame, but ignore 
that particular commit.

I’m guessing we could make this .git-blame-ignore-revs part of the tree, 
assuming though everyone must use git-cinnabar.

Jean-Yves



smime.p7s
Description: S/MIME cryptographic signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform