On Thu, Jun 29, 2017 at 12:30 AM, René Scharfe <l.s.r@xxxxxx> wrote:
> 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@xxxxxxxxx> 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@xxxxxxxxx>
>>>>>> ---
>>>>>
>>>>>
>>>>> 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@xxxxxx>
>>>> ---
>>>> 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?
At this point I have no idea what I did wrong, and no time/want to
really find out. But whatever it was was the wrong thing, obviously :)
