Message-ID: <Y5fdgI4uTpXZQ9yn@mail.google.com>
Date:   Tue, 13 Dec 2022 15:03:44 +1300
From:   Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@...il.com>
To:     Vipin Sharma <vipinsh@...gle.com>
Cc:     Masahiro Yamada <masahiroy@...nel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Nicolas Schier <nicolas@...sle.eu>,
        Jonathan Corbet <corbet@....net>,
        Cristian Ciocaltea <cristian.ciocaltea@...labora.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kbuild@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] scripts/tags.sh: choose which directories to exclude
 from being indexed

On Mon, Dec 12, 2022 at 02:32:41PM -0800, Vipin Sharma wrote:
> On Mon, Dec 12, 2022 at 1:59 PM Paulo Miguel Almeida
> <paulo.miguel.almeida.rodenas@...il.com> wrote:
> >
> > On Mon, Dec 12, 2022 at 01:27:37PM -0800, Vipin Sharma wrote:
> > > On Sat, Dec 10, 2022 at 3:02 PM Paulo Miguel Almeida
> > > <paulo.miguel.almeida.rodenas@...il.com> wrote:
> > > >  # find sources in rest of tree
> > > > -# we could benefit from a list of dirs to search in here
> > > >  find_other_sources()
> > > >  {
> > > > -       find ${tree}* $ignore \
> > > > +       local loc_ignore=${ignore}
> > > > +       if [ -n "${IGNOREDIRS}" ]; then
> > > > +               exp_ignored_dirs=$(sed 's/,/ /g' <<< ${IGNOREDIRS})
> > > > +               for i in ${exp_ignored_dirs}; do
> > > > +                       loc_ignore="${loc_ignore} ( -path $i ) -prune -o"
> > > > +               done
> > > > +       fi
> > > > +
> > >
> > > This should be global overwrite instead of just in this function.
> > > Before find_other_sources() is executed, this script finds files in
> > > arch directories. So, if you keep it local then those files cannot be
> > > excluded which makes execution of the command incorrect:
> > >
> > > make IGNOREDIRS=arch/x86 cscope
> > >
> >
> > Hi Vipin, thanks for taking the time to review this patch.
> >
> > I see where you are coming from. I was aware of the 'loophole' that the
> > current approach could have but, to be honest, I thought that there
> > would be very little use in being able to exclude arch/.*?/ files.
> >
> > The reason for that being that I thought the most common usage for this
> > feature would be to ignore folders within subsystems like drivers and
> > tools to ensure code navigation would be less 'messy'.
> 
> Yes, the original intent was to make driver code browsing less messy
> but if we are introducing an option we should adapt it for generic
> cases and correct the semantics.

Agreed.

> 
> >
> > Additionally, if we go with the global IGNOREDIRS approach you just
> > described, we could have some conflicting options too such as:
> >
> > make ALLSOURCE_ARCHS="x86 arm" IGNOREDIRS=arch/x86 cscope
> >
> 
> I don't think this is conflicting, to me it is more complementary.
> Above line shows get all code for x86 and arm but don't get x86 source
> code ("arch/x86/include" is fine). This can even be fine tuned to sub
> directories.
> 

That's a fair point. I had not thought about it that way. Thanks!

Will implement the changes when I get home.

> I just now noticed after seeing your command, ALLSOURCE_ARCHS take
> space separated values, whereas, IGNOREDIRS take comma separated
> values. They both should be in the same format, since ALLSOURCE_ARCHS
> was already there, it is better to change IGNOREDIRS.
> 
> Can you also change IGNOREDIRS to IGNORE_DIRS? It is much easier to
> read this way. Sorry, I should have said this in the  beginning.
> 

Yep, no problem! :-)

> > My 2 cents is that ALLSOURCE_ARCHS is already the mechanism for
> > excluding archs so it's 'okay' to keep IGNOREDIRS as is.
> >
> > Let me know your thoughts.
> >
> > Thanks!
> >
> > - Paulo A.
> >
> > > Above command will still index all of the code in arch/x86. Something
> > > like this will be better.
> > >
> > > --- a/scripts/tags.sh
> > > +++ b/scripts/tags.sh
> > > @@ -17,6 +17,13 @@ ignore="$(echo "$RCS_FIND_IGNORE" | sed 's|\\||g' )"
> > >  # tags and cscope files should also ignore MODVERSION *.mod.c files
> > >  ignore="$ignore ( -name *.mod.c ) -prune -o"
> > >
> > > +if [ -n "${IGNOREDIRS}" ]; then
> > > +       exp_ignored_dirs=$(sed 's/,/ /g' <<< ${IGNOREDIRS})
> > > +       for i in ${exp_ignored_dirs}; do
> > > +               ignore="${ignore} ( -path $i ) -prune -o"
> > > +       done
> > > +fi
> > > +
> > >  # Use make KBUILD_ABS_SRCTREE=1 {tags|cscope}
> > >  # to force full paths for a non-O= build
> > >  if [ "${srctree}" = "." -o -z "${srctree}" ]; then
> > > @@ -62,9 +69,9 @@ find_include_sources()
> > >  # we could benefit from a list of dirs to search in here
> > >  find_other_sources()
> > >  {
> > > -       find ${tree}* $ignore \
> > > -            \( -path ${tree}include -o -path ${tree}arch -o -name
> > > '.tmp_*' \) -prune -o \
> > > -              -name "$1" -not -type l -print;
> > > +       find ${tree}* ${ignore} \
> > > +               \( -path ${tree}include -o -path ${tree}arch -o -name
> > > '.tmp_*' \) -prune -o \
> > > +               -name "$1" -not -type l -print;
> > >  }
> > >
> > > We will still have to specify arch/x86 and arch/x86/include but this
> > > works and keeps the definition of IGNOREDIRS relatively correct.
> > >
> > >
> > > > +       find ${tree}* ${loc_ignore} \
> > > >              \( -path ${tree}include -o -path ${tree}arch -o -name '.tmp_*' \) -prune -o \
> > > >                -name "$1" -not -type l -print;
> > > >  }
> > > > --
> > > > 2.38.1
> > > >

Powered by blists - more mailing lists