[PATCH] Add utility to check for indentation in files or patches

[Simo via samba-technical]

Hi all,
given recent discussions I finally took some time to do what I wanted
to do for a long time.

This utilities, which I've simply stolen and adapted from MIT krb5, can
be used in commit hooks or individually to check for style issues in
commits or files.
I do not know if putting them under python/ is right, you (reviewer)
tell me.

Run:
$ python ./python/cstyle.py

Your latest commit or your current uncommitted code is checked.

Pass in a revision or a revision range and specific commits can be
checked at once:
Example:
$ python ./python/cstyle.py HEAD~10..HEAD

The style violations are output per file with the line and kind of
violation.

You can style check a whole file this way:
$ python ./python/cstyle-file.py $FILENAME
Or:
$ cat $FILENAME | python ./python/cstyle-file.py


Try to check the last 100 commits for an idea of what violations are
found.

HTH,
Simo.

[Andrew Bartlett via samba-technical]

Post by Simo via samba-technical
Hi all,
given recent discussions I finally took some time to do what I wanted
to do for a long time.
This utilities, which I've simply stolen and adapted from MIT krb5, can
be used in commit hooks or individually to check for style issues in
commits or files.
I do not know if putting them under python/ is right, you (reviewer)
tell me.

script/ would be a better location, python/ is for python libraries. 1

Post by Simo via samba-technical
$ python ./python/cstyle.py
Your latest commit or your current uncommitted code is checked.
Pass in a revision or a revision range and specific commits can be
$ python ./python/cstyle.py HEAD~10..HEAD
The style violations are output per file with the line and kind of
violation.
$ python ./python/cstyle-file.py $FILENAME
$ cat $FILENAME | python ./python/cstyle-file.py
Try to check the last 100 commits for an idea of what violations are
found.

Thanks Simo, I appreciate you taking the effort to help bring us to a
soution here.  

Is there a way to whitelist/blacklist file types and directories with
this tool?

We really don't want commits bouncing because they import a new heimdal
or update third_party for example, and it would be helpful to be able
to exclude some files both temporarily when (for example) moving old
code around.

Thanks,

Andrew Bartlett

[Douglas Bagnall via samba-technical]

Post by Andrew Bartlett via samba-technical
Is there a way to whitelist/blacklist file types and directories with
this tool?
We really don't want commits bouncing because they import a new heimdal
or update third_party for example, and it would be helpful to be able
to exclude some files both temporarily when (for example) moving old
code around.

If we're talking about a pre-commit hook here, we could make the
rejection message say something like:

Try again with

git commit --no-verify

if you are sure you are doing the right thing.


Douglas

[Simo via samba-technical]

Post by Douglas Bagnall via samba-technical

Post by Andrew Bartlett via samba-technical
Is there a way to whitelist/blacklist file types and directories with
this tool?
We really don't want commits bouncing because they import a new heimdal
or update third_party for example, and it would be helpful to be able
to exclude some files both temporarily when (for example) moving old
code around.

If we're talking about a pre-commit hook here, we could make the
Try again with
git commit --no-verify
if you are sure you are doing the right thing.

The hook can definitely say that, yes.

These utilities work independently from the hook, so it would be weird
to bake it into them.

Simo.

[Simo via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Simo via samba-technical
Hi all,
given recent discussions I finally took some time to do what I wanted
to do for a long time.
This utilities, which I've simply stolen and adapted from MIT krb5, can
be used in commit hooks or individually to check for style issues in
commits or files.
I do not know if putting them under python/ is right, you (reviewer)
tell me.

script/ would be a better location, python/ is for python libraries. 1

For some reason I thought we do not have a top level script/ ...

Post by Andrew Bartlett via samba-technical

Post by Simo via samba-technical
$ python ./python/cstyle.py
Your latest commit or your current uncommitted code is checked.
Pass in a revision or a revision range and specific commits can be
$ python ./python/cstyle.py HEAD~10..HEAD
The style violations are output per file with the line and kind of
violation.
$ python ./python/cstyle-file.py $FILENAME
$ cat $FILENAME | python ./python/cstyle-file.py
Try to check the last 100 commits for an idea of what violations are
found.

Thanks Simo, I appreciate you taking the effort to help bring us to a
soution here.
Is there a way to whitelist/blacklist file types and directories with
this tool?

The original tool was looking exclusively under src/ what do you think
if I exclude all third-party/ and source4/heimdal* ?

Post by Andrew Bartlett via samba-technical
We really don't want commits bouncing because they import a new heimdal
or update third_party for example, and it would be helpful to be able
to exclude some files both temporarily when (for example) moving old
code around.

I think we may need a way to push through commits anyway, but we can
definitely improve the tool until it is 90% useful, and then handle
cases as they come by.

Simo.

[Martin Schwenke via samba-technical]

On Sun, 08 Apr 2018 13:02:10 -0400, Simo via samba-technical

Post by Simo via samba-technical
given recent discussions I finally took some time to do what I wanted
to do for a long time.
This utilities, which I've simply stolen and adapted from MIT krb5, can
be used in commit hooks or individually to check for style issues in
commits or files.
I do not know if putting them under python/ is right, you (reviewer)
tell me.
$ python ./python/cstyle.py
Your latest commit or your current uncommitted code is checked.
Pass in a revision or a revision range and specific commits can be
$ python ./python/cstyle.py HEAD~10..HEAD

Sorry, this doesn't work for me:

$ python ./python/cstyle.py HEAD~10..HEAD
649d1837061 ctdb-daemon: Add testing environment variable CTDB_LOCAL_DAEMON_MODE
Traceback (most recent call last):
File "python/cstyle-file.py", line 312, in <module>
check_file(lines)
File "python/cstyle-file.py", line 244, in check_file
indented_lines = reindent(lines)
File "python/cstyle-file.py", line 65, in reindent
p = Popen(args, stdout=PIPE, stderr=PIPE)
File "/usr/lib/python2.7/subprocess.py", line 394, in __init__
errread, errwrite)
File "/usr/lib/python2.7/subprocess.py", line 1047, in _execute_child
raise child_exception
OSError: [Errno 2] No such file or directory

It just seems to blow up on some commits with a traceback. Can someone
please explain what I'm doing wrong?

peace & happiness,
martin

[Simo via samba-technical]

Post by Martin Schwenke via samba-technical
On Sun, 08 Apr 2018 13:02:10 -0400, Simo via samba-technical

Post by Simo via samba-technical
given recent discussions I finally took some time to do what I wanted
to do for a long time.
This utilities, which I've simply stolen and adapted from MIT krb5, can
be used in commit hooks or individually to check for style issues in
commits or files.
I do not know if putting them under python/ is right, you (reviewer)
tell me.
$ python ./python/cstyle.py
Your latest commit or your current uncommitted code is checked.
Pass in a revision or a revision range and specific commits can be
$ python ./python/cstyle.py HEAD~10..HEAD

$ python ./python/cstyle.py HEAD~10..HEAD
649d1837061 ctdb-daemon: Add testing environment variable CTDB_LOCAL_DAEMON_MODE
File "python/cstyle-file.py", line 312, in <module>
check_file(lines)
File "python/cstyle-file.py", line 244, in check_file
indented_lines = reindent(lines)
File "python/cstyle-file.py", line 65, in reindent
p = Popen(args, stdout=PIPE, stderr=PIPE)
File "/usr/lib/python2.7/subprocess.py", line 394, in __init__
errread, errwrite)
File "/usr/lib/python2.7/subprocess.py", line 1047, in _execute_child
raise child_exception
OSError: [Errno 2] No such file or directory
It just seems to blow up on some commits with a traceback. Can someone
please explain what I'm doing wrong?

Do you have 'indent' installed ?
I probably need to catch the exception better, I changed this part from
the original stuff, as it was using emacs ...

Post by Martin Schwenke via samba-technical
peace & happiness,
martin

[Martin Schwenke via samba-technical]

Post by Simo via samba-technical

Post by Martin Schwenke via samba-technical
It just seems to blow up on some commits with a traceback. Can someone
please explain what I'm doing wrong?

Do you have 'indent' installed ?
I probably need to catch the exception better, I changed this part from
the original stuff, as it was using emacs ...

I didn't but I do now. Much better! Thanks! :-)

A couple of things that we might not want:

* No space before or after binary operator

We have "good" examples in README.Coding without spaces around binary
operators. For example, in a for-loop and in a calculation involving
sizeof().

* Parenthesized return expression

We don't have a rule against this.

In all the simple cases, doing this is bad style. However, for a
complex expression it might provide clarity.

For example, in the same way that:

x = (y.foo == z.bar);

gains clarity from the parentheses, I think the following does too:

return (y.foo == z.bar);

Or, perhaps, the latter is against the guidelines because we should
be doing:

ok = (y.foo == z.bar);
return ok;

However, since we have access to y.foo and z.bar in a debugger, we
can easily check what the return value will be, so I think the
original is OK.

Not sure... :-)

peace & happiness,
martin

[Simo via samba-technical]

Post by Martin Schwenke via samba-technical

Post by Simo via samba-technical

Post by Martin Schwenke via samba-technical
It just seems to blow up on some commits with a traceback. Can someone
please explain what I'm doing wrong?

Do you have 'indent' installed ?
I probably need to catch the exception better, I changed this part from
the original stuff, as it was using emacs ...

I didn't but I do now. Much better! Thanks! :-)
* No space before or after binary operator
We have "good" examples in README.Coding without spaces around binary
operators. For example, in a for-loop and in a calculation involving
sizeof().
* Parenthesized return expression
We don't have a rule against this.
In all the simple cases, doing this is bad style. However, for a
complex expression it might provide clarity.
x = (y.foo == z.bar);
return (y.foo == z.bar);
Or, perhaps, the latter is against the guidelines because we should
ok = (y.foo == z.bar);
return ok;
However, since we have access to y.foo and z.bar in a debugger, we
can easily check what the return value will be, so I think the
original is OK.
Not sure... :-)

Thanks,
I am updating the code to work with python3 (the original catered for
python 2.6 ...) and I will add measures to deal with your feedback too,
although I have to say I think I may prefer to keep both rules above as
that's what indent also does ?
Otherwise I need to check how to suppress that in indent I guess.

But I'll look into it.

Simo.

[Simo via samba-technical]

Attached find a new version that deals with python3 properly and
also has better handling of indent, by spitting only a diff file with
the propsed diff you should apply to pass it's rules.
Previously the code was trying to line match and give a generic error
with the line number but that doesn't work because indent can change
lines by wrapping or unwrapping so the line numbers would make no sense
on longer files.

I have no yet addressed the space and parenthesized issues in this
specific email simply because I ran out of time and I am not sure we
actually do not want them.

I am open to changing these though if more people agree on the need for
changing them.

Simo.

Post by Andrew Bartlett via samba-technical

Post by Martin Schwenke via samba-technical

Post by Simo via samba-technical

Post by Martin Schwenke via samba-technical
It just seems to blow up on some commits with a traceback. Can someone
please explain what I'm doing wrong?

Do you have 'indent' installed ?
I probably need to catch the exception better, I changed this part from
the original stuff, as it was using emacs ...

I didn't but I do now. Much better! Thanks! :-)
* No space before or after binary operator
We have "good" examples in README.Coding without spaces around binary
operators. For example, in a for-loop and in a calculation involving
sizeof().
* Parenthesized return expression
We don't have a rule against this.
In all the simple cases, doing this is bad style. However, for a
complex expression it might provide clarity.
x = (y.foo == z.bar);
return (y.foo == z.bar);
Or, perhaps, the latter is against the guidelines because we should
ok = (y.foo == z.bar);
return ok;
However, since we have access to y.foo and z.bar in a debugger, we
can easily check what the return value will be, so I think the
original is OK.
Not sure... :-)

Thanks,
I am updating the code to work with python3 (the original catered for
python 2.6 ...) and I will add measures to deal with your feedback too,
although I have to say I think I may prefer to keep both rules above as
that's what indent also does ?
Otherwise I need to check how to suppress that in indent I guess.
But I'll look into it.
Simo.

[Andrew Bartlett via samba-technical]

Post by Simo via samba-technical
Attached find a new version that deals with python3 properly and
also has better handling of indent, by spitting only a diff file with
the propsed diff you should apply to pass it's rules.
Previously the code was trying to line match and give a generic error
with the line number but that doesn't work because indent can change
lines by wrapping or unwrapping so the line numbers would make no sense
on longer files.
I have no yet addressed the space and parenthesized issues in this
specific email simply because I ran out of time and I am not sure we
actually do not want them.
I am open to changing these though if more people agree on the need for
changing them.

G'Day Simo,

Thank you so much for taking charge of this.

I wondered if the check-file script could be re-worked into a subunit test?

It would be great if we had an idea how much of our code actually
complies with README.Coding.

I'm thinking that if we wrote a rule to run the check-file script
against each .c and .h file in the tree, encoding the file name and
rule into the test output we could then handle exceptions via our skip,
knownfail and flapping process.

I'm hoping that would make it easier to add exceptions when files are
renamed or functions re-ordered, without absolutely requiring that the
code be re-formatted there and then.

If we actually tested the full tree we could perhaps fix up some of the
more egregious examples, particularly where it is just whitespace (and
so can be verified as harmless with a whitespace-ignoring diff).

Thanks!

Andrew Bartlett

Post by Simo via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Martin Schwenke via samba-technical

Post by Simo via samba-technical

Post by Martin Schwenke via samba-technical
It just seems to blow up on some commits with a traceback. Can someone
please explain what I'm doing wrong?

Do you have 'indent' installed ?
I probably need to catch the exception better, I changed this part from
the original stuff, as it was using emacs ...

I didn't but I do now. Much better! Thanks! :-)
* No space before or after binary operator
We have "good" examples in README.Coding without spaces around binary
operators. For example, in a for-loop and in a calculation involving
sizeof().
* Parenthesized return expression
We don't have a rule against this.
In all the simple cases, doing this is bad style. However, for a
complex expression it might provide clarity.
x = (y.foo == z.bar);
return (y.foo == z.bar);
Or, perhaps, the latter is against the guidelines because we should
ok = (y.foo == z.bar);
return ok;
However, since we have access to y.foo and z.bar in a debugger, we
can easily check what the return value will be, so I think the
original is OK.
Not sure... :-)

Thanks,
I am updating the code to work with python3 (the original catered for
python 2.6 ...) and I will add measures to deal with your feedback too,
although I have to say I think I may prefer to keep both rules above as
that's what indent also does ?
Otherwise I need to check how to suppress that in indent I guess.
But I'll look into it.
Simo.

[Simo via samba-technical]

On Tue, 2018-04-10 at 10:30 +1200, Andrew Bartlett via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Simo via samba-technical
Attached find a new version that deals with python3 properly and
also has better handling of indent, by spitting only a diff file with
the propsed diff you should apply to pass it's rules.
Previously the code was trying to line match and give a generic error
with the line number but that doesn't work because indent can change
lines by wrapping or unwrapping so the line numbers would make no sense
on longer files.
I have no yet addressed the space and parenthesized issues in this
specific email simply because I ran out of time and I am not sure we
actually do not want them.
I am open to changing these though if more people agree on the need for
changing them.

G'Day Simo,
Thank you so much for taking charge of this.
I wondered if the check-file script could be re-worked into a subunit test?

I do not know, check-file can check a whole file, but I am not sure we
want to do it right away.

Post by Andrew Bartlett via samba-technical
It would be great if we had an idea how much of our code actually
complies with README.Coding.

You can do it easily with a find/xargs script that calls check-file.py
on each file, so far I haven't found a single file that complies :-)

Post by Andrew Bartlett via samba-technical
I'm thinking that if we wrote a rule to run the check-file script
against each .c and .h file in the tree, encoding the file name and
rule into the test output we could then handle exceptions via our skip,
knownfail and flapping process.

Keep in mind we do not check the whole file on commit, we check the
diff, so that existing violations are not flagged.

Post by Andrew Bartlett via samba-technical
I'm hoping that would make it easier to add exceptions when files are
renamed or functions re-ordered, without absolutely requiring that the
code be re-formatted there and then.

I think it is a good idea to always have style correction on file
renames and function reorders (as separate commits of course), it
allows us to gradually fix our code.

Making exceptions for those is not as easy, and I am not sure how the
implementation would look like for function moves (file renames are
probably easy enough).

Post by Andrew Bartlett via samba-technical
If we actually tested the full tree we could perhaps fix up some of the
more egregious examples, particularly where it is just whitespace (and
so can be verified as harmless with a whitespace-ignoring diff).

I think we can try to achieve these goals, but as follow up work, I
would like to start having something now rather than try to shoot for
everything at once.

Simo.

Post by Andrew Bartlett via samba-technical
Thanks!
Andrew Bartlett

Post by Simo via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Martin Schwenke via samba-technical

Post by Simo via samba-technical

Post by Martin Schwenke via samba-technical
It just seems to blow up on some commits with a traceback. Can someone
please explain what I'm doing wrong?

Do you have 'indent' installed ?
I probably need to catch the exception better, I changed this part from
the original stuff, as it was using emacs ...

I didn't but I do now. Much better! Thanks! :-)
* No space before or after binary operator
We have "good" examples in README.Coding without spaces around binary
operators. For example, in a for-loop and in a calculation involving
sizeof().
* Parenthesized return expression
We don't have a rule against this.
In all the simple cases, doing this is bad style. However, for a
complex expression it might provide clarity.
x = (y.foo == z.bar);
return (y.foo == z.bar);
Or, perhaps, the latter is against the guidelines because we should
ok = (y.foo == z.bar);
return ok;
However, since we have access to y.foo and z.bar in a debugger, we
can easily check what the return value will be, so I think the
original is OK.
Not sure... :-)

Thanks,
I am updating the code to work with python3 (the original catered for
python 2.6 ...) and I will add measures to deal with your feedback too,
although I have to say I think I may prefer to keep both rules above as
that's what indent also does ?
Otherwise I need to check how to suppress that in indent I guess.
But I'll look into it.
Simo.

[Andrew Bartlett via samba-technical]

Post by Simo via samba-technical

Post by Andrew Bartlett via samba-technical
If we actually tested the full tree we could perhaps fix up some of the
more egregious examples, particularly where it is just whitespace (and
so can be verified as harmless with a whitespace-ignoring diff).

I think we can try to achieve these goals, but as follow up work, I
would like to start having something now rather than try to shoot for
everything at once.

I totally agree, just signalling a direction I would like us to go, and
perhaps seeing if you might be interested in helping us get there.

Thanks,

Andrew Bartlett

[Simo via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Simo via samba-technical

Post by Andrew Bartlett via samba-technical
If we actually tested the full tree we could perhaps fix up some of the
more egregious examples, particularly where it is just whitespace (and
so can be verified as harmless with a whitespace-ignoring diff).

I think we can try to achieve these goals, but as follow up work, I
would like to start having something now rather than try to shoot for
everything at once.

I totally agree, just signalling a direction I would like us to go, and
perhaps seeing if you might be interested in helping us get there.

I am, time permitting.

Simo.

[Martin Schwenke via samba-technical]

Hi Simo,

Post by Simo via samba-technical
Attached find a new version that deals with python3 properly and
also has better handling of indent, by spitting only a diff file with
the propsed diff you should apply to pass it's rules.
Previously the code was trying to line match and give a generic error
with the line number but that doesn't work because indent can change
lines by wrapping or unwrapping so the line numbers would make no sense
on longer files.

Looks good! I notice that it doesn't exit with non-zero when it
(thinks that it) finds issues in my current branch. Do we want it to
do that? Would that be as simple as having warn() set a global?

Post by Simo via samba-technical
I have no yet addressed the space and parenthesized issues in this
specific email simply because I ran out of time and I am not sure we
actually do not want them.
I am open to changing these though if more people agree on the need for
changing them.

I don't think that we should error on things that README.Coding doesn't
prohibit, particularly if they appear in "good" examples. I think this
is particularly true if people are pushing hard for this code to be
used in tests to validate code. It would be wrong to have tests that
contradict README.Coding. We could, of course, change README.Coding if
there is a good reason to do so.

I hope that others agree... :-)

I note that these particular checks seem to be implemented in
cstyle-file.py (via check_binary_operator() and
check_parenthesized_return()), so they should be easy to remove.

peace & happiness,
martin

[Simo via samba-technical]

On Tue, 2018-04-10 at 10:27 +1000, Martin Schwenke via samba-technical

Post by Martin Schwenke via samba-technical
Hi Simo,

Post by Simo via samba-technical
Attached find a new version that deals with python3 properly and
also has better handling of indent, by spitting only a diff file with
the propsed diff you should apply to pass it's rules.
Previously the code was trying to line match and give a generic error
with the line number but that doesn't work because indent can change
lines by wrapping or unwrapping so the line numbers would make no sense
on longer files.

Looks good! I notice that it doesn't exit with non-zero when it
(thinks that it) finds issues in my current branch. Do we want it to
do that? Would that be as simple as having warn() set a global?

No I think we want to exit with an error when it failed to check stuff,
a commit hook would rather decide based on the output ?
But we can change that later once we actually propose a hook if we find
it better to deal with it via return errors.

Post by Martin Schwenke via samba-technical

Post by Simo via samba-technical
I have no yet addressed the space and parenthesized issues in this
specific email simply because I ran out of time and I am not sure we
actually do not want them.
I am open to changing these though if more people agree on the need for
changing them.

I don't think that we should error on things that README.Coding doesn't
prohibit, particularly if they appear in "good" examples. I think this
is particularly true if people are pushing hard for this code to be
used in tests to validate code. It would be wrong to have tests that
contradict README.Coding. We could, of course, change README.Coding if
there is a good reason to do so.

It is not easy to figure out exactly what is required, we also suggest
using indent, but I already had to add one more indent option to the
ones we mention there to account for the fact we keep labels on the
beginning of the line and do not indent them.

Post by Martin Schwenke via samba-technical
I hope that others agree... :-)

I would definitely like someone else to add feedback or RB+

Post by Martin Schwenke via samba-technical
I note that these particular checks seem to be implemented in
cstyle-file.py (via check_binary_operator() and
check_parenthesized_return()), so they should be easy to remove.

I think we could perhaps gather them into an optional step, but the
current code does not allow for adding options.
I can disable them if someone else wants to comment.

Wouldn't want to go back and forth too many times.

Simo.

[Simo via samba-technical]

Post by Simo via samba-technical

Post by Martin Schwenke via samba-technical
On Sun, 08 Apr 2018 13:02:10 -0400, Simo via samba-technical

Post by Simo via samba-technical
given recent discussions I finally took some time to do what I wanted
to do for a long time.
This utilities, which I've simply stolen and adapted from MIT krb5, can
be used in commit hooks or individually to check for style issues in
commits or files.
I do not know if putting them under python/ is right, you (reviewer)
tell me.
$ python ./python/cstyle.py
Your latest commit or your current uncommitted code is checked.
Pass in a revision or a revision range and specific commits can be
$ python ./python/cstyle.py HEAD~10..HEAD

$ python ./python/cstyle.py HEAD~10..HEAD
649d1837061 ctdb-daemon: Add testing environment variable CTDB_LOCAL_DAEMON_MODE
File "python/cstyle-file.py", line 312, in <module>
check_file(lines)
File "python/cstyle-file.py", line 244, in check_file
indented_lines = reindent(lines)
File "python/cstyle-file.py", line 65, in reindent
p = Popen(args, stdout=PIPE, stderr=PIPE)
File "/usr/lib/python2.7/subprocess.py", line 394, in __init__
errread, errwrite)
File "/usr/lib/python2.7/subprocess.py", line 1047, in _execute_child
raise child_exception
OSError: [Errno 2] No such file or directory
It just seems to blow up on some commits with a traceback. Can someone
please explain what I'm doing wrong?

Do you have 'indent' installed ?
I probably need to catch the exception better, I changed this part from
the original stuff, as it was using emacs ...

It won't work right regardless, sorry, fixing and adding other feedback
, will post followup soon.

Simo.