Re: [PATCH v5 06/21] perf script: Move find_scripts to browser/scripts.c

[Ian Rogers <irogers@google.com>]

On Mon, Nov 4, 2024 at 11:47 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Oct 31, 2024 at 01:51:36PM -0700, Ian Rogers wrote:
> > On Thu, Oct 31, 2024 at 12:18 PM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > On Wed, Oct 30, 2024 at 06:42:37PM -0700, Ian Rogers wrote:
> > > > The only use of find_scripts is in browser/scripts.c but the
> > > > definition in builtin causes linking problems requiring a stub in
> > > > python.c. Move the function to allow the stub to be removed.
> > > >
> > > > Rewrite the directory iteration to use openat so that large character
> > > > arrays aren't needed. The arrays are warned about potential buffer
> > > > overflows by GCC now that all the code exists in a single C file.
> > >
> > > Introducing is_directory_at() should be done as a prep patch, as the
> > > rest of the patch below could end up being reverted after some other
> > > patch used it, making the process more difficult.
> > >
> > > I mentioned cases like this in the past, so doing it again just for the
> > > record.
> >
> > This is highlighted in the commit message:
> > ```
> > Rewrite the directory iteration to use openat so that large character
> > arrays aren't needed. The arrays are warned about potential buffer
> > overflows by GCC now that all the code exists in a single C file.
> > ```
> > so without the change the code wouldn't build. The new is_directory_at
> > function is effectively 2 statements fstatat and S_ISDIR on the
> > result, it is put next to is_directory for consistency but could have
> > been a static function in the only C file to use it.
> >
> > For the record, patches introducing 2 line long functions can be
> > excessively noisy, especially in a 21 patch series. There is always
> > the declared but not used build error to worry about - here things
> > couldn't just be simply moved due to triggering a different build
> > error. Given the simplicity of the function here I made a decision not
> > to split up the work - the commit message would likely be longer than
> > the function. The work never intended to introduce is_directory_at but
> > was forced into it through a desire not to disable compiler warnings.
>
> This patch does more than just moving the code which can be easy to miss
> something in the middle.  I think you can move the code as is without
> introducing build errors and then add new changes like using openat() on
> top (you may separate the change out of this series).  I think it's
> ok to have a small change if it clearly has different semantics.

If you are trying to bisect to find something that broke a build,
having commits that knowingly break the build will cause the bisect to
fail. The bisect will falsely fail on the known to be broken commit.
It should be unacceptable to knowingly break the build in a commit for
this reason.

Thanks,
Ian