Re: [PATCH 01/23] contrib/coccinnelle: add equals-null.cocci

From: Philip Oakley <philipoakley@iee.email>
To: Elia Pinto <gitter.spiros@gmail.com>, Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 01/23] contrib/coccinnelle: add equals-null.cocci
Date: Sun, 1 May 2022 18:22:07 +0100	[thread overview]
Message-ID: <d996dc46-ae9a-6506-8043-0ac5468d9dd4@iee.email> (raw)
In-Reply-To: <CA+EOSBnx3-G02=zXGUrRuKPTDPBSYoBY=rERCORe8NtywEOiGg@mail.gmail.com>

On 01/05/2022 18:04, Elia Pinto wrote:
> Il giorno dom 1 mag 2022 alle ore 02:20 Junio C Hamano
> <gitster@pobox.com> ha scritto:
>> What I found curious is that the result of applying these patches to
>> v2.36.0 and running coccicheck reveals that we are not making the
>> codebase clean wrt this new coccinelle rule.
>>
> It is possible, I did not use coccicheck to apply the semantic patch
> (on next)  but i use a my script which I think is slightly more
> efficient but perhaps it is not so correct. Anyway, given the
> discussion that has taken place so far, what do you think is best for
> me to do? Do a reroll (perhaps with only 2 patches in total ) or wait
> for the "right" moment in the future as foreseen by the Documentation
> and dedicate the time to more useful contributions for git? Thank you
> all for the review
>
Hi Elia,

Given Junio's comment regarding the potential for patch churn, it may,
as an alternative, be possible to create a script that will
check/extract just those those changes that are either:

A) mistakes in patches between say master and seen `master..seen` so
that they aren't incorporated, though `next..seen` may be more appropriate.

B) detect 'while you are at it' fixes that could be applied to a file
that is being modified with the same range query. The file fix-up patch
could/would then be inserted as preparatory step to the relevant series
(i.e. fed to the series author), allowing easy reversion with/without
the fix-up.

C) suggest to coccinelle that maybe they include a similar Git based
range check option, to avoid needing a fancy script [using Junio's
rationale].

D) more clearly identify _why_ the particular instances that are
corrected are _worth_ changing now (e.g. fixing _one_ may be a worthy
GSOC or Outreachy activity, or .. <insert own reasons>).

 Hope that helps.

Philip

>> diff -u -p a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c
>> --- a/compat/fsmonitor/fsm-listen-darwin.c
>> +++ b/compat/fsmonitor/fsm-listen-darwin.c
>> @@ -342,7 +342,7 @@ int fsm_listen__ctor(struct fsmonitor_da
>>                                            data->cfar_paths_to_watch,
>>                                            kFSEventStreamEventIdSinceNow,
>>                                            0.001, flags);
>> -       if (data->stream == NULL)
>> +       if (!data->stream)
>>                 goto failed;
[snipped]