Re: [PATCH v2 2/6] test-fast-rebase helper: use release_revisions() (again)

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Jul 29, 2022 at 4:33 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Fix a bug in 0139c58ab95 (revisions API users: add "goto cleanup" for
> release_revisions(), 2022-04-13), in that commit a release_revisions()
> call was added to this function, but it never did anything due to this
> TODO memset() added in fe1a21d5267 (fast-rebase: demonstrate
> merge-ort's API via new test-tool command, 2020-10-29).
>
> Simply removing the memset() will fix the "cmdline" which can be seen
> when running t5520-pull.sh.
>
> This sort of thing could be detected automatically with a rule similar
> to the unused.cocci merged in 7fa60d2a5b6 (Merge branch
> 'ab/cocci-unused' into next, 2022-07-11). The following rule on top
> would catch the case being fixed here:
>
>         @@
>         type T;
>         identifier I;
>         identifier REL1 =~ "^[a-z_]*_(release|reset|clear|free)$";
>         identifier REL2 =~ "^(release|clear|free)_[a-z_]*$";
>         @@
>
>         - memset(\( I \| &I \), 0, ...);
>           ... when != \( I \| &I \)
>         (
>           \( REL1 \| REL2 \)( \( I \| &I \), ...);
>         |
>           \( REL1 \| REL2 \)( \( &I \| I \) );
>         )
>           ... when != \( I \| &I \)
>
> That rule should arguably use only &I, not I (as we might be passed a
> pointer). He distinction would matter if anyone cared about the

s/He/The/

> side-effects of a memset() followed by release() of a pointer to a
> variable passed into the function.
>
> As such a pattern would be at best very confusing, and most likely
> point to buggy code as in this case, the above rule is probably fine
> as-is.
>
> But as this rule only found one such bug in the entire codebase let's
> not add it to contrib/coccinelle/unused.cocci for now, we can always
> dig it up in the future if it's deemed useful.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>