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

[Evan Huus <eapache@xxxxxxxxx>]

On Fri, Jul 11, 2014 at 12:42 PM, Bill Meier <wmeier@xxxxxxxxxxx> wrote:

I've been working with the current 'pre-commit' and have noticed the following issues:

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 ?
         - ???

     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').

   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.

2. In addition, I propose to add calls to checkhf and fix-encoding-args
   under the make file checkapi targets for dissector files.

====

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.

When you try and push a change to gerrit without a Commit-ID, the error message it returns includes the Commit-ID it's expecting, so you can manually git commit --amend and paste that into the footer.

 

[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@wireshark.org?subject=unsubscribe