Disallow pushing of new trailing whitespace

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Disallow pushing of new trailing whitespace

Jan Stolarek
Right now we have a git hook that prevents pushing a file containing tabs, unless that file had them already (in other words: no new files with tabs in our repos). I propose to add similar hook for trailing whitespaces. Herbert says he can implement that. What do others think? Would you find that useful or problematic?

Janek



Reply | Threaded
Open this post in threaded view
|

Disallow pushing of new trailing whitespace

Geoffrey Mainland
Would be nice to have. How about a third hook that disallows commits
that include whitespace-only changes unless *all* changes are
whitespace-only? ;)

Geoff

On 08/20/2013 10:59 AM, Jan Stolarek wrote:
> Right now we have a git hook that prevents pushing a file containing tabs, unless that file had them already (in other words: no new files with tabs in our repos). I propose to add similar hook for trailing whitespaces. Herbert says he can implement that. What do others think? Would you find that useful or problematic?
>
> Janek



Reply | Threaded
Open this post in threaded view
|

Disallow pushing of new trailing whitespace

Herbert Valerio Riedel
On 2013-08-20 at 13:21:02 +0200, Geoffrey Mainland wrote:
> How about a third hook that disallows commits that include
> whitespace-only changes unless *all* changes are whitespace-only? ;)

The other two validations were about preserving an invariant ("file has
no tabs" & "file has no trailing whitespace") and more or less simple to
decide;

...whereas your suggestion seems problematic as it's difficult to know
whether a whitespace change to a Haskell program has semantic meaning;
for example, how should the script detect that the following whitespace
modification...

--- main.hs 2013-08-20 14:53:34.119960468 +0200
+++ main2.hs 2013-08-20 14:53:43.295960294 +0200
@@ -1,5 +1,5 @@
 foo x = do
     putStrLn "foo"
     when x $ do
       putStrLn "bar"
-      putStrLn "doo"
+    putStrLn "doo"


...is actually a semantic change?

Cheers,
  hvr



Reply | Threaded
Open this post in threaded view
|

Disallow pushing of new trailing whitespace

Geoffrey Mainland
I wasn't seriously suggesting such a hook...thus the wink.

I *am* seriously suggesting that whitespace changes not be mixed with
other changes in a given commit.

Geoff

On 08/20/2013 01:55 PM, Herbert Valerio Riedel wrote:

> On 2013-08-20 at 13:21:02 +0200, Geoffrey Mainland wrote:
>> How about a third hook that disallows commits that include
>> whitespace-only changes unless *all* changes are whitespace-only? ;)
> The other two validations were about preserving an invariant ("file has
> no tabs" & "file has no trailing whitespace") and more or less simple to
> decide;
>
> ...whereas your suggestion seems problematic as it's difficult to know
> whether a whitespace change to a Haskell program has semantic meaning;
> for example, how should the script detect that the following whitespace
> modification...
>
> --- main.hs 2013-08-20 14:53:34.119960468 +0200
> +++ main2.hs 2013-08-20 14:53:43.295960294 +0200
> @@ -1,5 +1,5 @@
>  foo x = do
>      putStrLn "foo"
>      when x $ do
>        putStrLn "bar"
> -      putStrLn "doo"
> +    putStrLn "doo"
>
>
> ...is actually a semantic change?
>
> Cheers,
>   hvr
>




Reply | Threaded
Open this post in threaded view
|

Disallow pushing of new trailing whitespace

Simon Marlow-7
In reply to this post by Geoffrey Mainland
On 20/08/13 12:21, Geoffrey Mainland wrote:
> Would be nice to have. How about a third hook that disallows commits
> that include whitespace-only changes unless *all* changes are
> whitespace-only? ;)

The problem with these kind of commit-time checks is that you only find
out the problem *after* you've validated your nicely polished commits.
If we're going to add more checks on commits they should be done by
validate (yes I know that's not at all trivial, but it's not impossible
either).

Cheers,
        Simon



> Geoff
>
> On 08/20/2013 10:59 AM, Jan Stolarek wrote:
>> Right now we have a git hook that prevents pushing a file containing tabs, unless that file had them already (in other words: no new files with tabs in our repos). I propose to add similar hook for trailing whitespaces. Herbert says he can implement that. What do others think? Would you find that useful or problematic?
>>
>> Janek
>
> _______________________________________________
> ghc-devs mailing list
> ghc-devs at haskell.org
> http://www.haskell.org/mailman/listinfo/ghc-devs
>




Reply | Threaded
Open this post in threaded view
|

Disallow pushing of new trailing whitespace

Austin Seipp-4
This seems acceptable IMO. The general working conventions already are to
separate whitespace and/or tab changes from a commit containing actual
content. If you ./validate cleanly, but the server rejects the push for
whitespace, adding an extra commit on top to clean up the affected files
seems very sensible (it's simple to 'untabify' and eliminate trailing white
space in most editors these days, so I imagine this isn't really going to
cost you a lot of time.) I also add bonus points for the fact the server
will reject it unless you clean up *all* affected files you touched, so
things can't slip by. This is how the tab check works now, and it does deal
with a 'range' of commits where later commits eliminate tabs introduced in
earlier ones.

We'd also have to introduce the concept of git into ./validate for this to
work (and add the ability to specify a commit range for stuff like this,)
but a basic implementation may not be difficult, looking at the pre-receive
script.

Alternatively, these could be pre-commit hooks in the developer repository,
but I believe you have to opt into that. Maybe worth putting on the wiki,
though.



On Thu, Aug 22, 2013 at 2:33 AM, Simon Marlow <marlowsd at gmail.com> wrote:

> On 20/08/13 12:21, Geoffrey Mainland wrote:
>
>> Would be nice to have. How about a third hook that disallows commits
>> that include whitespace-only changes unless *all* changes are
>> whitespace-only? ;)
>>
>
> The problem with these kind of commit-time checks is that you only find
> out the problem *after* you've validated your nicely polished commits. If
> we're going to add more checks on commits they should be done by validate
> (yes I know that's not at all trivial, but it's not impossible either).
>
> Cheers,
>         Simon
>
>
>
>
>  Geoff
>>
>> On 08/20/2013 10:59 AM, Jan Stolarek wrote:
>>
>>> Right now we have a git hook that prevents pushing a file containing
>>> tabs, unless that file had them already (in other words: no new files with
>>> tabs in our repos). I propose to add similar hook for trailing whitespaces.
>>> Herbert says he can implement that. What do others think? Would you find
>>> that useful or problematic?
>>>
>>> Janek
>>>
>>
>> ______________________________**_________________
>> ghc-devs mailing list
>> ghc-devs at haskell.org
>> http://www.haskell.org/**mailman/listinfo/ghc-devs<http://www.haskell.org/mailman/listinfo/ghc-devs>
>>
>>
>
> ______________________________**_________________
> ghc-devs mailing list
> ghc-devs at haskell.org
> http://www.haskell.org/**mailman/listinfo/ghc-devs<http://www.haskell.org/mailman/listinfo/ghc-devs>
>



--
Regards,
Austin - PGP: 4096R/0x91384671
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.haskell.org/pipermail/ghc-devs/attachments/20130822/329b3e44/attachment.htm>

Reply | Threaded
Open this post in threaded view
|

Disallow pushing of new trailing whitespace

Simon Marlow-7
On 22/08/13 12:32, Austin Seipp wrote:
> This seems acceptable IMO. The general working conventions already are
> to separate whitespace and/or tab changes from a commit containing
> actual content. If you ./validate cleanly, but the server rejects the
> push for whitespace, adding an extra commit on top to clean up the
> affected files seems very sensible (it's simple to 'untabify' and
> eliminate trailing white space in most editors these days, so I imagine
> this isn't really going to cost you a lot of time.)

You still have to validate the new commit, so it costs you at least 20
mins, which is a lot.

> I also add bonus
> points for the fact the server will reject it unless you clean up *all*
> affected files you touched, so things can't slip by. This is how the tab
> check works now, and it does deal with a 'range' of commits where later
> commits eliminate tabs introduced in earlier ones.
>
> We'd also have to introduce the concept of git into ./validate for this
> to work (and add the ability to specify a commit range for stuff like
> this,) but a basic implementation may not be difficult, looking at the
> pre-receive script.

Checking everything in 'git rev-list origin..' is probably good enough.

Cheers,
        Simon



> Alternatively, these could be pre-commit hooks in the developer
> repository, but I believe you have to opt into that. Maybe worth putting
> on the wiki, though.
>
>
>
> On Thu, Aug 22, 2013 at 2:33 AM, Simon Marlow <marlowsd at gmail.com
> <mailto:marlowsd at gmail.com>> wrote:
>
>     On 20/08/13 12:21, Geoffrey Mainland wrote:
>
>         Would be nice to have. How about a third hook that disallows commits
>         that include whitespace-only changes unless *all* changes are
>         whitespace-only? ;)
>
>
>     The problem with these kind of commit-time checks is that you only
>     find out the problem *after* you've validated your nicely polished
>     commits. If we're going to add more checks on commits they should be
>     done by validate (yes I know that's not at all trivial, but it's not
>     impossible either).
>
>     Cheers,
>              Simon
>
>
>
>
>         Geoff
>
>         On 08/20/2013 10:59 AM, Jan Stolarek wrote:
>
>             Right now we have a git hook that prevents pushing a file
>             containing tabs, unless that file had them already (in other
>             words: no new files with tabs in our repos). I propose to
>             add similar hook for trailing whitespaces. Herbert says he
>             can implement that. What do others think? Would you find
>             that useful or problematic?
>
>             Janek
>
>
>         _________________________________________________
>         ghc-devs mailing list
>         ghc-devs at haskell.org <mailto:ghc-devs at haskell.org>
>         http://www.haskell.org/__mailman/listinfo/ghc-devs
>         <http://www.haskell.org/mailman/listinfo/ghc-devs>
>
>
>
>     _________________________________________________
>     ghc-devs mailing list
>     ghc-devs at haskell.org <mailto:ghc-devs at haskell.org>
>     http://www.haskell.org/__mailman/listinfo/ghc-devs
>     <http://www.haskell.org/mailman/listinfo/ghc-devs>
>
>
>
>
> --
> Regards,
> Austin - PGP: 4096R/0x91384671




Reply | Threaded
Open this post in threaded view
|

Disallow pushing of new trailing whitespace

Edward Z. Yang
GHC already has -fwarn-tabs; we could have -fwarn-trailing-whitespace
and turn it on by default, so that validate errors on it but you also
notice it when you're doing a build (if you're paying attention to
warnings).

Edward

Excerpts from Simon Marlow's message of Thu Aug 22 05:04:50 -0700 2013:

> On 22/08/13 12:32, Austin Seipp wrote:
> > This seems acceptable IMO. The general working conventions already are
> > to separate whitespace and/or tab changes from a commit containing
> > actual content. If you ./validate cleanly, but the server rejects the
> > push for whitespace, adding an extra commit on top to clean up the
> > affected files seems very sensible (it's simple to 'untabify' and
> > eliminate trailing white space in most editors these days, so I imagine
> > this isn't really going to cost you a lot of time.)
>
> You still have to validate the new commit, so it costs you at least 20
> mins, which is a lot.
>
> > I also add bonus
> > points for the fact the server will reject it unless you clean up *all*
> > affected files you touched, so things can't slip by. This is how the tab
> > check works now, and it does deal with a 'range' of commits where later
> > commits eliminate tabs introduced in earlier ones.
> >
> > We'd also have to introduce the concept of git into ./validate for this
> > to work (and add the ability to specify a commit range for stuff like
> > this,) but a basic implementation may not be difficult, looking at the
> > pre-receive script.
>
> Checking everything in 'git rev-list origin..' is probably good enough.
>
> Cheers,
>     Simon
>
> > Alternatively, these could be pre-commit hooks in the developer
> > repository, but I believe you have to opt into that. Maybe worth putting
> > on the wiki, though.
> >
> >
> >
> > On Thu, Aug 22, 2013 at 2:33 AM, Simon Marlow <marlowsd at gmail.com
> > <mailto:marlowsd at gmail.com>> wrote:
> >
> >     On 20/08/13 12:21, Geoffrey Mainland wrote:
> >
> >         Would be nice to have. How about a third hook that disallows commits
> >         that include whitespace-only changes unless *all* changes are
> >         whitespace-only? ;)
> >
> >
> >     The problem with these kind of commit-time checks is that you only
> >     find out the problem *after* you've validated your nicely polished
> >     commits. If we're going to add more checks on commits they should be
> >     done by validate (yes I know that's not at all trivial, but it's not
> >     impossible either).
> >
> >     Cheers,
> >              Simon
> >
> >
> >
> >
> >         Geoff
> >
> >         On 08/20/2013 10:59 AM, Jan Stolarek wrote:
> >
> >             Right now we have a git hook that prevents pushing a file
> >             containing tabs, unless that file had them already (in other
> >             words: no new files with tabs in our repos). I propose to
> >             add similar hook for trailing whitespaces. Herbert says he
> >             can implement that. What do others think? Would you find
> >             that useful or problematic?
> >
> >             Janek
> >
> >
> >         _________________________________________________
> >         ghc-devs mailing list
> >         ghc-devs at haskell.org <mailto:ghc-devs at haskell.org>
> >         http://www.haskell.org/__mailman/listinfo/ghc-devs
> >         <http://www.haskell.org/mailman/listinfo/ghc-devs>
> >
> >
> >
> >     _________________________________________________
> >     ghc-devs mailing list
> >     ghc-devs at haskell.org <mailto:ghc-devs at haskell.org>
> >     http://www.haskell.org/__mailman/listinfo/ghc-devs
> >     <http://www.haskell.org/mailman/listinfo/ghc-devs>
> >
> >
> >
> >
> > --
> > Regards,
> > Austin - PGP: 4096R/0x91384671
>



Reply | Threaded
Open this post in threaded view
|

Disallow pushing of new trailing whitespace

Simon Marlow-7
There's some justification for -fwarn-tabs: tabs can lead to confusing
failures when people use the wrong tab setting in their editor.  On the
other hand, I don't think there's any good reason for having the
compiler warn about trailing whitespace.

Cheers,
        Simon

On 22/08/13 17:18, Edward Z. Yang wrote:

> GHC already has -fwarn-tabs; we could have -fwarn-trailing-whitespace
> and turn it on by default, so that validate errors on it but you also
> notice it when you're doing a build (if you're paying attention to
> warnings).
>
> Edward
>
> Excerpts from Simon Marlow's message of Thu Aug 22 05:04:50 -0700 2013:
>> On 22/08/13 12:32, Austin Seipp wrote:
>>> This seems acceptable IMO. The general working conventions already are
>>> to separate whitespace and/or tab changes from a commit containing
>>> actual content. If you ./validate cleanly, but the server rejects the
>>> push for whitespace, adding an extra commit on top to clean up the
>>> affected files seems very sensible (it's simple to 'untabify' and
>>> eliminate trailing white space in most editors these days, so I imagine
>>> this isn't really going to cost you a lot of time.)
>>
>> You still have to validate the new commit, so it costs you at least 20
>> mins, which is a lot.
>>
>>> I also add bonus
>>> points for the fact the server will reject it unless you clean up *all*
>>> affected files you touched, so things can't slip by. This is how the tab
>>> check works now, and it does deal with a 'range' of commits where later
>>> commits eliminate tabs introduced in earlier ones.
>>>
>>> We'd also have to introduce the concept of git into ./validate for this
>>> to work (and add the ability to specify a commit range for stuff like
>>> this,) but a basic implementation may not be difficult, looking at the
>>> pre-receive script.
>>
>> Checking everything in 'git rev-list origin..' is probably good enough.
>>
>> Cheers,
>>      Simon
>>
>>> Alternatively, these could be pre-commit hooks in the developer
>>> repository, but I believe you have to opt into that. Maybe worth putting
>>> on the wiki, though.
>>>
>>>
>>>
>>> On Thu, Aug 22, 2013 at 2:33 AM, Simon Marlow <marlowsd at gmail.com
>>> <mailto:marlowsd at gmail.com>> wrote:
>>>
>>>      On 20/08/13 12:21, Geoffrey Mainland wrote:
>>>
>>>          Would be nice to have. How about a third hook that disallows commits
>>>          that include whitespace-only changes unless *all* changes are
>>>          whitespace-only? ;)
>>>
>>>
>>>      The problem with these kind of commit-time checks is that you only
>>>      find out the problem *after* you've validated your nicely polished
>>>      commits. If we're going to add more checks on commits they should be
>>>      done by validate (yes I know that's not at all trivial, but it's not
>>>      impossible either).
>>>
>>>      Cheers,
>>>               Simon
>>>
>>>
>>>
>>>
>>>          Geoff
>>>
>>>          On 08/20/2013 10:59 AM, Jan Stolarek wrote:
>>>
>>>              Right now we have a git hook that prevents pushing a file
>>>              containing tabs, unless that file had them already (in other
>>>              words: no new files with tabs in our repos). I propose to
>>>              add similar hook for trailing whitespaces. Herbert says he
>>>              can implement that. What do others think? Would you find
>>>              that useful or problematic?
>>>
>>>              Janek
>>>
>>>
>>>          _________________________________________________
>>>          ghc-devs mailing list
>>>          ghc-devs at haskell.org <mailto:ghc-devs at haskell.org>
>>>          http://www.haskell.org/__mailman/listinfo/ghc-devs
>>>          <http://www.haskell.org/mailman/listinfo/ghc-devs>
>>>
>>>
>>>
>>>      _________________________________________________
>>>      ghc-devs mailing list
>>>      ghc-devs at haskell.org <mailto:ghc-devs at haskell.org>
>>>      http://www.haskell.org/__mailman/listinfo/ghc-devs
>>>      <http://www.haskell.org/mailman/listinfo/ghc-devs>
>>>
>>>
>>>
>>>
>>> --
>>> Regards,
>>> Austin - PGP: 4096R/0x91384671
>>




Reply | Threaded
Open this post in threaded view
|

Disallow pushing of new trailing whitespace

Edward Z. Yang
Yes, I suppose this is a philosophical difference of whether or not
the compiler should enforce coding style. :)

Edward

Excerpts from Simon Marlow's message of Sat Aug 24 13:21:30 -0700 2013:

> There's some justification for -fwarn-tabs: tabs can lead to confusing
> failures when people use the wrong tab setting in their editor.  On the
> other hand, I don't think there's any good reason for having the
> compiler warn about trailing whitespace.
>
> Cheers,
>     Simon
>
> On 22/08/13 17:18, Edward Z. Yang wrote:
> > GHC already has -fwarn-tabs; we could have -fwarn-trailing-whitespace
> > and turn it on by default, so that validate errors on it but you also
> > notice it when you're doing a build (if you're paying attention to
> > warnings).
> >
> > Edward
> >
> > Excerpts from Simon Marlow's message of Thu Aug 22 05:04:50 -0700 2013:
> >> On 22/08/13 12:32, Austin Seipp wrote:
> >>> This seems acceptable IMO. The general working conventions already are
> >>> to separate whitespace and/or tab changes from a commit containing
> >>> actual content. If you ./validate cleanly, but the server rejects the
> >>> push for whitespace, adding an extra commit on top to clean up the
> >>> affected files seems very sensible (it's simple to 'untabify' and
> >>> eliminate trailing white space in most editors these days, so I imagine
> >>> this isn't really going to cost you a lot of time.)
> >>
> >> You still have to validate the new commit, so it costs you at least 20
> >> mins, which is a lot.
> >>
> >>> I also add bonus
> >>> points for the fact the server will reject it unless you clean up *all*
> >>> affected files you touched, so things can't slip by. This is how the tab
> >>> check works now, and it does deal with a 'range' of commits where later
> >>> commits eliminate tabs introduced in earlier ones.
> >>>
> >>> We'd also have to introduce the concept of git into ./validate for this
> >>> to work (and add the ability to specify a commit range for stuff like
> >>> this,) but a basic implementation may not be difficult, looking at the
> >>> pre-receive script.
> >>
> >> Checking everything in 'git rev-list origin..' is probably good enough.
> >>
> >> Cheers,
> >>      Simon
> >>
> >>> Alternatively, these could be pre-commit hooks in the developer
> >>> repository, but I believe you have to opt into that. Maybe worth putting
> >>> on the wiki, though.
> >>>
> >>>
> >>>
> >>> On Thu, Aug 22, 2013 at 2:33 AM, Simon Marlow <marlowsd at gmail.com
> >>> <mailto:marlowsd at gmail.com>> wrote:
> >>>
> >>>      On 20/08/13 12:21, Geoffrey Mainland wrote:
> >>>
> >>>          Would be nice to have. How about a third hook that disallows commits
> >>>          that include whitespace-only changes unless *all* changes are
> >>>          whitespace-only? ;)
> >>>
> >>>
> >>>      The problem with these kind of commit-time checks is that you only
> >>>      find out the problem *after* you've validated your nicely polished
> >>>      commits. If we're going to add more checks on commits they should be
> >>>      done by validate (yes I know that's not at all trivial, but it's not
> >>>      impossible either).
> >>>
> >>>      Cheers,
> >>>               Simon
> >>>
> >>>
> >>>
> >>>
> >>>          Geoff
> >>>
> >>>          On 08/20/2013 10:59 AM, Jan Stolarek wrote:
> >>>
> >>>              Right now we have a git hook that prevents pushing a file
> >>>              containing tabs, unless that file had them already (in other
> >>>              words: no new files with tabs in our repos). I propose to
> >>>              add similar hook for trailing whitespaces. Herbert says he
> >>>              can implement that. What do others think? Would you find
> >>>              that useful or problematic?
> >>>
> >>>              Janek
> >>>
> >>>
> >>>          _________________________________________________
> >>>          ghc-devs mailing list
> >>>          ghc-devs at haskell.org <mailto:ghc-devs at haskell.org>
> >>>          http://www.haskell.org/__mailman/listinfo/ghc-devs
> >>>          <http://www.haskell.org/mailman/listinfo/ghc-devs>
> >>>
> >>>
> >>>
> >>>      _________________________________________________
> >>>      ghc-devs mailing list
> >>>      ghc-devs at haskell.org <mailto:ghc-devs at haskell.org>
> >>>      http://www.haskell.org/__mailman/listinfo/ghc-devs
> >>>      <http://www.haskell.org/mailman/listinfo/ghc-devs>
> >>>
> >>>
> >>>
> >>>
> >>> --
> >>> Regards,
> >>> Austin - PGP: 4096R/0x91384671
> >>



Reply | Threaded
Open this post in threaded view
|

Disallow pushing of new trailing whitespace

Jan Stolarek
In reply to this post by Simon Marlow-7
> The problem with these kind of commit-time checks is that you only find
> out the problem *after* you've validated your nicely polished commits.
This is easily solved by adding this line to .emacs file:

(add-hook 'before-save-hook 'delete-trailing-whitespace)

No more trailing whitespaces. Ever.

On the other hand problem with NOT having these kind of checks is that I'm seeing some trailing whitespaces appearing in modules that I have cleaned from trailing whitespaces! Which means that I have to spend time with `git add -i` and separating whitespace changes from other changes (or ignore policy about whitespace changes going in a separate commit).

Janek



Reply | Threaded
Open this post in threaded view
|

Disallow pushing of new trailing whitespace

Mikhail Glushenkov-2
Hi,

On Thu, Aug 29, 2013 at 12:52 PM, Jan Stolarek <jan.stolarek at p.lodz.pl> wrote:
>> The problem with these kind of commit-time checks is that you only find
>> out the problem *after* you've validated your nicely polished commits.
> This is easily solved by adding this line to .emacs file:
>
> (add-hook 'before-save-hook 'delete-trailing-whitespace)
>
> No more trailing whitespaces. Ever.

The problem with this approach is that if a file already contains
trailing whitespace, you'll introduce spurious changes in your commit.
This can make patches quite hard to review. One solution is to use
ethan-wspace [1].

[1] https://github.com/glasserc/ethan-wspace



Reply | Threaded
Open this post in threaded view
|

Disallow pushing of new trailing whitespace

Jan Stolarek
> The problem with this approach is that if a file already contains
> trailing whitespace, you'll introduce spurious changes in your commit.
I wouldn't call it a problem, but a feature - provided that you put whitespace changes in a separate commit. In this way we would gradually get rid of all trailing whitespaces and live happily ever after. Right now I'm facing a dilemma whether I should use mentioned emacs setting or not. If I do use it, I end up removing lots of whitespaces simply when I edit files (and I'm fine with that), but I don't know if there's any point in doing this if they keep reappearing. I can then disable this setting, but I know I'll end up adding even more whitespaces to the source code and I don't think that's a good thing to do.

Janek