Re: [PATCH] test-progress: fix test failures on big-endian systems

On Mon, Oct 21, 2019 at 09:52:11AM +0900, Junio C Hamano wrote:

I agree, we should wait for the first real use case where specifying a larger-than-32bit integer actually makes sense in practice.

The semantic patch below will do that, but this is one of those "I don't have the slightest idea what I am doing" patches...

It's output looks like this when applied to an older version without the big-endian fix upthread:

  potential error at apply.c:4982:26:
    passing variable 'state -> p_context' of type 'unsigned int' to OPT_INTEGER
    OPT_INTEGER expects an int
  potential error at builtin/column.c:29:30:
    passing variable 'colopts' of type 'unsigned int' to OPT_INTEGER
    OPT_INTEGER expects an int
  potential error at builtin/column.c:32:24:
    passing variable 'copts . nl' of type 'const char *' to OPT_INTEGER
    OPT_INTEGER expects an int
  potential error at builtin/grep.c:884:38:
    passing variable 'opt . pre_context' of type 'unsigned' to OPT_INTEGER
    OPT_INTEGER expects an int
  potential error at builtin/grep.c:886:37:
    passing variable 'opt . post_context' of type 'unsigned' to OPT_INTEGER
    OPT_INTEGER expects an int
  potential error at builtin/upload-pack.c:28:29:
    passing variable 'opts . timeout' of type 'unsigned int' to OPT_INTEGER
    OPT_INTEGER expects an int
  potential error at t/helper/test-progress.c:42:27:
    passing variable 'total' of type 'uint64_t' to OPT_INTEGER
    OPT_INTEGER expects an int

  https://travis-ci.org/szeder/git/jobs/602423358#L436

I think most of them are harmless, like the number of context lines in apply and grep, or the timeout in seconds in upload-pack. So I think the semantic patch should allow 'unsigned' and 'unsigned int' as well.

But note the one in 'builtin/column.c', where we pass a 'const char *' to OPT_INTEGER. That can't possibly be good; I suspect copy-paste error and it should have been OPT_STRING.

 --- >8 ---

Subject: [PATCH] coccinelle: warn about passing a non-int to parse-options'
 OPT_INTEGER

parse-options' OPT_INTEGER wants to parse an integer argument into a variable of type 'int', and passing e.g. an 'uint64_t' causes troubles [1].

Add a Coccinelle semantic patch that checks the type of the variable where the integer argument should be parsed into, and print an error if that variable is not of type 'int'.

Note that this semantic patch won't result in a proper and applicable patch, because who knows where that variable of the inappropriate type is defined. However, the printed error message will still cause our static analysis CI jobs to fail, drawing our attention to the issue.

TODO: refusing an 'unsigned int' might be unnecessarily harsh...

[1] 11a803d861 (test-progress: fix test failures on big-endian
    systems, 2019-10-20)

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/coccinelle/parse-options.cocci | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 contrib/coccinelle/parse-options.cocci