Wireshark · Wireshark-dev: Re: [Wireshark-dev] Current 'pre-commit' issues

[Alexis La Goutte <alexis.lagoutte@xxxxxxxxx>]

Hi Bill,

On Fri, Jul 11, 2014 at 6:42 PM, Bill Meier <wmeier@xxxxxxxxxxx> wrote:
> I've been working with the current 'pre-commit' and have noticed the
> following issues:

at origin, pre-commit script is personnal script to easy launch some
check tools ;-) and there is a lot of issue...

>
> 1.  Using the current pre-commit which calls checkAPIs, etc, it
>     doesn't seem possible to make changes to
>     certain files (e.g., wsgetopt.c) and submit them to Gerrit.
>
>     - The files fail checkAPIs.pl as called from pre-commit
>       (Error: deprecated/prohibited APIs).
>
>       'git commit --no-verify' bypasses pre-commit, but also bypasses
>       commit-msg and thus the commit message doesn't
>       have a Change-ID and thus is rejected by Gerrit. [**See below]
>
>         Is there a way around this (short of temporarily removing
>         the pre-commit script) ?
>          - somehow generate a Gerrit Commit-ID manually ?
>          - ???
it is now fix by Evan and if i remember it is a rand sha1 start with I...
>
>      I note that there a a number of (non-dissector) files
>      which fail the default checkAPIs. Many of the Errors could probably
>      be fixed, but some look either OK or a bit tricky.
>
>      The reason that we don't see these checkAPIs issues with
>      'make checkapi' is that the checkAPIs for the files which fail
>      have been commented out (thus sort of saying the Errors are OK).
>
> 2. checkhf and fix-encoding-args are being called for all files (not
>    just dissectors).
>
>
> ====
>
> 1. For the above reasons, I propose that pre-commit only do checkAPIs,
>    checkhf and fix-encoding-args for dissector files (to be determined
>    in a rather ugly ad-hoc way by seeing if the file is named
>    "packet-.+\.[hc]" (as is done now with 'checkAPIs -p').
Yes good idea !
>
>    pre-commit would still do the whitespace check for all files.
>
>    checkAPIs can be called for all .[hc] files when/if the current
>    Errors are fixed.
need to add "exception" for ui/qt about C comment because // is a
valid comment in C++ and checkAPI fail in ui/qt (there is a small hack
in pre-commit check only c and h files but there is some h files in
ui/qt...)

Also there is a other issue when remove file (script try always to
check remove file...)

And i will be nice if it is possible to check the last commit (or a
specificity commit id)
>
> 2. In addition, I propose to add calls to checkhf and fix-encoding-args
>    under the make file checkapi targets for dissector files.
Good idea too
>
> ====
>
> Thoughts ?
>
>
> Bill
>
>
>
>
> [**]
>
> The Wireshark wiki [1] states
>
> "If you must have the whitespace as it is, you can run git commit
> --no-verify to avoid the pre-commit and commit-msg hooks. Note: using
> --no-verify avoids the commit-msg hook, and thus does not add a
>    Change-ID automatically to your commit."
>
> Ok: We really don't want to accept commits which don't pass the whitespace
> test so this shouldn't be an issue when using
> the default pre-commit.
>
> However, the Wiki doesn't say what to do when we really, really
> "must have the whitespace" except to say '--no-verify' leaves
> us with a commit message with no Commit-ID.
>
>
> [1] http://wiki.wireshark.org/Development/SubmittingPatches
> ___________________________________________________________________________
> Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
> Archives:    http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>             mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe