Code checker hook script

[Balazs Dezso]

The [e7a4f5ef2250] contains a script to check code conformance, which can be used as commit hook in hg. It also contains a minor improvement in unify-sources script.

[Balazs Dezso]

The [00e953d56f15] patch contains a cleaned version of the commit. Please apply it instead of the old one.

[Peter Kovacs]

I cannot import [00e953d56f15] with --exact.

[Peter Kovacs]

Replying to kpeter:

I cannot import [00e953d56f15] with --exact.

Otherwise I like this script, it helps me a lot.

Btw. shouldn't these two scripts check the Makefile.am files, too? (Of course they should not have header.) I think it would be important, since there were some trouble with whitespace changes in these files, too.

[Peter Kovacs]

also check Makefile.am files

[Peter Kovacs]

Replying to kpeter:

Btw. shouldn't these two scripts check the Makefile.am files, too? (Of course they should not have header.) I think it would be important, since there were some trouble with whitespace changes in these files, too.

[043f5a75c230] makes the scripts check these files, too (except for the header checking).

I suggest to import [00e953d56f15] to the main branch (or a modified variant of it, since it cannot be imported with --exact), because it is a useful tool. If [043f5a75c230] is also accepted then these changesets could be merged into one changeset, and unify_sources.sh should be applied to the repository.

[Balazs Dezso]

The [a8f15f9907db] is a merge of the previous two patches.

[Alpar Juttner]

Replying to deba:

The [a8f15f9907db] is a merge of the previous two patches.

There are tho issues here.

1. The executable flag is missing from scripts/check-sources.sh
2. scripts/check-sources.sh is not compatible with scripts/unify-sources.sh. If I run scripts/check-sources.sh on [a8f15f9907db] it says everything is ok, but scripts/unify-sources.sh does several changes at the same time.

[Peter Kovacs]

Replying to alpar:

1. The executable flag is missing from scripts/check-sources.sh

I can't create such a patch that contains this information. If I add the executable flag and export the changes, the created patch can't be imported with --exact.

2. scripts/check-sources.sh is not compatible with scripts/unify-sources.sh. If I run scripts/check-sources.sh on [a8f15f9907db] it says everything is ok, but scripts/unify-sources.sh does several changes at the same time.

This is because the two scripts are designed to use for different purposes. check-sources.sh is mainly for using as a hg hook, that is why it checks only the modified files when called without an argument. However [3701beec445a] contains an improved version, in which check-sources.sh can be called with the --all (or -a) option to check all the sources files as unify-sources.sh does.

[Alpar Juttner]

Replying to kpeter:

Replying to alpar:

1. The executable flag is missing from scripts/check-sources.sh

I can't create such a patch that contains this information.

You can, you just probably don't know how. :)

If I add the executable flag and export the changes, the created patch can't be imported with --exact.

The --git option is your friend. I strongly suggest to use it by default by placing something like this into ~/.hgrc.

[diff]
git=1
[defaults]
qimport = --git
qrefresh = --git

This is from my own hg config file. It may be that [diff] section is enough for everything, I haven't checked it.

[Alpar Juttner]

Replying to kpeter:

Btw. shouldn't these two scripts check the Makefile.am files, too?

Makefile.am files should not be checked/modified in this way. The tabs in Makefiles are mandatory and should not be replaced, even if GNU's make allows to do that.

[Peter Kovacs]

Replying to alpar:

The --git option is your friend. I strongly suggest to use it by default by placing something like this into ~/.hgrc.

Thank you. [158ef3b71f5c] is the fixed patch.

[Peter Kovacs]

Replying to alpar:

Makefile.am files should not be checked/modified in this way. The tabs in Makefiles are mandatory and should not be replaced, even if GNU's make allows to do that.

I understand, but the unification is necessary in any case. See e.g. lemon/Makefile.am​. TABs should be replaced to 8 spaces or 8 spaces should be replaced to TABs.

[Balazs Dezso]

Replying to kpeter:

Replying to alpar:

1. The executable flag is missing from scripts/check-sources.sh

I can't create such a patch that contains this information. If I add the executable flag and export the changes, the created patch can't be imported with --exact.

2. scripts/check-sources.sh is not compatible with scripts/unify-sources.sh. If I run scripts/check-sources.sh on [a8f15f9907db] it says everything is ok, but scripts/unify-sources.sh does several changes at the same time.

This is because the two scripts are designed to use for different purposes. check-sources.sh is mainly for using as a hg hook, that is why it checks only the modified files when called without an argument. However [3701beec445a] contains an improved version, in which check-sources.sh can be called with the --all (or -a) option to check all the sources files as unify-sources.sh does.

Fortunately, the check-sources.sh checks the commited files, not all the files. This is the correct behaviour of the hook. In the script file there is a comment about how the script should be used. The executable flag should be added.

Unfortnately, the hgrc cannot be copied automically with clone (for example if I make a local clone, and I would like to add the same hgrc to each lemon repository, but there are no hooks for local clones), so the hook config should be write into each repository.

[Peter Kovacs]

Replying to deba:

Unfortnately, the hgrc cannot be copied automically with clone (for example if I make a local clone, and I would like to add the same hgrc to each lemon repository, but there are no hooks for local clones), so the hook config should be write into each repository.

Another option is to write the hook config into the "global" .hgrc file, so it is used for all local repositories.

[Alpar Juttner]

Replying to kpeter:

Another option is to write the hook config into the "global" .hgrc file, so it is used for all local repositories.

Which is an option only if you never use hg anything else but for LEMON development. I would suggest the following instead.

1. Setup a repository called out for your outgoing changesets and install the script on incoming (and not the commit) hook of this repo.
2. Whenever you starts working on something, you clone the main branch (of which you should also keep a local copy)
3. Instead of directly committing, use MQ for creating changesets, for it makes it easy to modify them later.
4. When you have finished your work, push it to the outgoing repo. It is fails, edit the wrong files, refresh the changeset with hg qref and push it again.

[Peter Kovacs]

What should we do with this ticket? Alpar do not like replacing TABs with 8 spaces in Makefile.am files. However unification would be necessary in these files, too. See e.g. lemon/Makefile.am​.

So every 8 spaces should be replaced with TABs or all TABs should be replaced with 8 spaces.

I think we have to options:

• unify these files manually or
• extend the unification script to these files.

[Alpar Juttner]

Replying to kpeter:

What should we do with this ticket? Alpar do not like replacing TABs with 8 spaces in Makefile.am files.

This is not a matter of taste but it is because a standard makefile requires TABs not spaces.

In general I support unification, but I do not consider it very urgent. If we have patch for the hook script and the unifier that also handles Makefile, that is nice, but I don't think it is a blocker of release 1.0.

[Peter Kovacs]

Updated version of Balazs's original changeset [00e953d56f15]

[Peter Kovacs]

Improvements (also check & unify Makefile.am files correctly)

[Peter Kovacs]

I cleaned the script files. They check and unify the Makefile.am files according to Alpar's notes.

Please review [1437eaaa5a5b] and [80edda6f89cd]. If they will be applied, I also suggest to run the unification script on the repository (it will unify Makefile.am files and two header files). Maybe it would be better before the release of Lemon 1.0.

[Peter Kovacs]

What do you think about [1437eaaa5a5b] and [80edda6f89cd]?

[Alpar Juttner]

Replying to kpeter:

What do you think about [1437eaaa5a5b] and [80edda6f89cd]?

Here are my comments

• I think it's a bad idea to have too scripts for unifying the source. It doubles the maintenance work and it is also error prone. The checker script should be just an option of the unify script. Currently it first creates a unified version of the source files in a temp file and then overwrites the original one with it. For checking the source, it would be enough to diff these files instead of overwriting.
• Unfortunately long lines are sometimes unavoidable in makefiles and in doxygen documentations, thus these files should not be checked for long lines.

[Peter Kovacs]

Replying to alpar:

Here are my comments

• I think it's a bad idea to have too scripts for unifying the source. It doubles the maintenance work and it is also error prone. The checker script should be just an option of the unify script. Currently it first creates a unified version of the source files in a temp file and then overwrites the original one with it. For checking the source, it would be enough to diff these files instead of overwriting.
  
• Unfortunately long lines are sometimes unavoidable in makefiles and in doxygen documentations, thus these files should not be checked for long lines.

[f0ce608e320e] contains a reworked version according to your comments. For more information run the script with --help option.

[Balazs Dezso]

Replying to kpeter:

Replying to alpar:

Here are my comments

• I think it's a bad idea to have too scripts for unifying the source. It doubles the maintenance work and it is also error prone. The checker script should be just an option of the unify script. Currently it first creates a unified version of the source files in a temp file and then overwrites the original one with it. For checking the source, it would be enough to diff these files instead of overwriting.
  
• Unfortunately long lines are sometimes unavoidable in makefiles and in doxygen documentations, thus these files should not be checked for long lines.

[f0ce608e320e] contains a reworked version according to your comments. For more information run the script with --help option.

I would like to suggest some improvements on this patch.

• In my point of view, that which files are used and what we do with the files are two independent things, therefore the parameters should be separate
• The different file selection methods could be the following, all files, the modified files in the repo, the given files in command line, and the commited files just for hook usage.
• The effect could be modification, check, and check with long line query.
• Do not use magic numbers in scripts, use rather strings.

[Peter Kovacs]

Replying to deba:

I would like to suggest some improvements on this patch.

I agree with your ideas. But could you please help me implementing them?

[Alpar Juttner]

Replying to deba:

• In my point of view, that which files are used and what we do with the files are two independent things, therefore the parameters should be separate

I agree.

• The different file selection methods could be the following, all files, the modified files in the repo, the given files in command line, and the commited files just for hook usage.

These flags can be -a, -m and nothing (i.e. the files are explicitly given), respectively.

• The effect could be modification, check, and check with long line query.

I suggest a -n or --dry-run option to change nothing just checking.

The long lines should only be checked on .h and .cc files, and they should always be checked on them IMHO. So an option for that is not so important. Also long lines cannot be broken automatically, but they should be reported, of course.

• Do not use magic numbers in scripts, use rather strings.

What do "magic numbers" mean here?

[Alpar Juttner]

Replying to kpeter:

I agree with your ideas. But could you please help me implementing them?

I will do that next week. (Assuming that I will learn what are "magic numbers" we want to get rid of.)

[Peter Kovacs]

Replying to alpar:

I will do that next week. (Assuming that I will learn what are "magic numbers" we want to get rid of.)

Thank you.

Magic numbers: I think it refers to the usage of the CHECK_MODE variable I introduced. This variable can have values of {0,1,2}.

[Peter Kovacs]

Replying to alpar:

• The different file selection methods could be the following, all files, the modified files in the repo, the given files in command line, and the commited files just for hook usage.

These flags can be -a, -m and nothing (i.e. the files are explicitly given), respectively.

And what about checking only the committed files? I suggest --all|-a, --modified|-m, --committed|-c, and nothing for the files that are given as command line arguments.

• The effect could be modification, check, and check with long line query.

I suggest a -n or --dry-run option to change nothing just checking.

A prefer --nomodify|-n.

[Alpar Juttner]

Replying to kpeter:

I suggest a -n or --dry-run option to change nothing just checking.

A prefer --nomodify|-n.

--dry-run notation is quite widely used, see e.g. svn help merge

[Balazs Dezso]

Improved unifier script

[Balazs Dezso]

The [e05633b02e40] contains the improved unifier and checker script.

[Peter Kovacs]

Small improvements for [e05633b02e40]

[Peter Kovacs]

Print line numbers (depends on [cdbff91c2166])

[Peter Kovacs]

Replying to deba:

The [e05633b02e40] contains the improved unifier and checker script.

I really like it. This version is much better than all of the older ones! The new --interactive|-i and --werror|-w options are also useful.

I just made two changesets to improve it:

• [cdbff91c2166] contains small improvements.
• [d900fd1e760f] adds line number display ability for the checking mode, since I missed it in the new implementation.

[Alpar Juttner]

Replying to kpeter:

I just made two changesets to improve it:

• [cdbff91c2166] contains small improvements.
• [d900fd1e760f] adds line number display ability for the checking mode, since I missed it in the new implementation.

Meanwhile I also made some improvements, see [0fbbb4bc42dd]. All of them are in the main branch now.