Re: [PATCH v4 4/6] coccinelle: add a rule to make "expression" code use FREE_AND_NULL()

> Am 29.06.2017 um 00:21 schrieb René Scharfe:
>>
>> Am 28.06.2017 um 23:39 schrieb Ævar Arnfjörð Bjarmason:
>>>
>>>
>>> On Sun, Jun 25 2017, René Scharfe jotted:
>>>
>>>> Am 16.06.2017 um 21:43 schrieb Junio C Hamano:
>>>>>
>>>>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>>>>
>>>>>> A follow-up to the existing "type" rule added in an earlier
>>>>>> change. This catches some occurrences that are missed by the previous
>>>>>> rule.
>>>>>>
>>>>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>>>>> ---
>>>>>
>>>>>
>>>>> Hmph, I wonder if the "type" thing is really needed.  Over there,
>>>>> "ptr" is an expression and we can find "free(ptr); ptr = NULL" with
>>>>> the rule in this patch already, no?
>>>>
>>>>
>>>> Indeed.  How about this on top of master?
>>>>
>>>> -- >8 --
>>>> Subject: [PATCH] coccinelle: polish FREE_AND_NULL rules
>>>>
>>>> There are two rules for using FREE_AND_NULL in free.cocci, one for
>>>> pointer types and one for expressions.  Both cause coccinelle to remove
>>>> empty lines and even newline characters between replacements for some
>>>> reason; consecutive "free(x);/x=NULL;" sequences end up as multiple
>>>> FREE_AND_NULL calls on the same time.
>>>>
>>>> Remove the type rule, as the expression rule already covers it, and
>>>> rearrange the lines of the latter to place the addition of FREE_AND_NULL
>>>> between the removals, which causes coccinelle to leave surrounding
>>>> whitespace untouched.
>>>>
>>>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>>>> ---
>>>>    contrib/coccinelle/free.cocci | 10 +---------
>>>>    1 file changed, 1 insertion(+), 9 deletions(-)
>>>>
>>>> diff --git a/contrib/coccinelle/free.cocci
>>>> b/contrib/coccinelle/free.cocci
>>>> index f2d97e755b..4490069df9 100644
>>>> --- a/contrib/coccinelle/free.cocci
>>>> +++ b/contrib/coccinelle/free.cocci
>>>> @@ -11,16 +11,8 @@ expression E;
>>>>      free(E);
>>>>
>>>>    @@
>>>> -type T;
>>>> -T *ptr;
>>>> -@@
>>>> -- free(ptr);
>>>> -- ptr = NULL;
>>>> -+ FREE_AND_NULL(ptr);
>>>> -
>>>> -@@
>>>>    expression E;
>>>>    @@
>>>>    - free(E);
>>>> -- E = NULL;
>>>>    + FREE_AND_NULL(E);
>>>> +- E = NULL;
>>>
>>>
>>> Late reply, sorry. What version of coccinelle are you running? I have
>>> 1.0.4 (from Debian) and can't get this to produce the same results as
>>> what I have.
>>>
>>> On top of next, I did:
>>>
>>>           Revert "*.[ch] refactoring: make use of the FREE_AND_NULL()
>>> macro"
>>>           Revert "coccinelle: make use of the "expression"
>>> FREE_AND_NULL() rule"
>>>           Revert "coccinelle: make use of the "type" FREE_AND_NULL()
>>> rule"
>>>
>>> And then generated the patch as usual with `make coccicheck`, and
>>> applied it. This has your change.
>>>
>>> I then re-applied the manual "*.[ch] refactoring" changes
>>>
>>> This results in this diff with next:
>>>
>>>       $ git diff --stat origin/next.. -- '*.[ch]'
>>>        builtin/am.c             |  3 ++-
>>>        builtin/clean.c          |  6 ++++--
>>>        builtin/config.c         |  6 ++++--
>>>        builtin/index-pack.c     |  6 ++++--
>>>        builtin/pack-objects.c   | 12 ++++++++----
>>>        builtin/unpack-objects.c |  3 ++-
>>>        fast-import.c            |  6 ++++--
>>>        http-push.c              | 24 ++++++++++++++++--------
>>>        http.c                   | 15 ++++++++++-----
>>>        imap-send.c              |  3 ++-
>>>        ref-filter.c             |  3 ++-
>>>        refs/files-backend.c     |  3 ++-
>>>        remote-testsvn.c         |  3 ++-
>>>        sequencer.c              |  3 ++-
>>>        sha1-array.c             |  3 ++-
>>>        sha1_file.c              |  3 ++-
>>>        transport-helper.c       | 27 ++++++++++++++++++---------
>>>        transport.c              |  3 ++-
>>>        tree-diff.c              |  6 ++++--
>>>        tree.c                   |  3 ++-
>>>        20 files changed, 94 insertions(+), 47 deletions(-)
>>>
>>> These are all cases where we now miss things that should use
>>> FREE_AND_NULL(), e.g.:
>>>
>>>       diff --git a/builtin/am.c b/builtin/am.c
>>>       index c973bd96dc..2f89338ed7 100644
>>>       --- a/builtin/am.c
>>>       +++ b/builtin/am.c
>>>       @@ -484,7 +484,8 @@ static int run_applypatch_msg_hook(struct
>>> am_state *state)
>>>               ret = run_hook_le(NULL, "applypatch-msg", am_path(state,
>>> "final-commit"), NULL);
>>>
>>>               if (!ret) {
>>>       -               FREE_AND_NULL(state->msg);
>>>       +               free(state->msg);
>>>       +               state->msg = NULL;
>>>                       if (read_commit_msg(state) < 0)
>>>                               die(_("'%s' was deleted by the
>>> applypatch-msg hook"),
>>>                                       am_path(state, "final-commit"));
>>>
>>> So it looks to me like removing the "type T" rule breaks a lot of
>>> things, but that the change you made to the expression rule is good, and
>>> we should do that for the "type" rule as well. Your commit says the
>>> "expression rule already covers it", but this doesn't seem to be the
>>> case at all.
>>>
>>> As an aside: Junio, did you mean to apply f8bb4631fb to next this way?
>>> Looks like a mis-applied scissor commit.
>>
>>
>> I can't reproduce that strange result with master, but get a scissors
>> line into the commit message as well.  The diff at the end looks
>> reasonable (contains just the manual stuff).  Here's what I did:
>
>
> On top of next it looks basically the same for me, only difference
> being several new converted cases in repository.c.
>
> Did you commit before diff?