Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <Y5T66yWNVAZNIaJ0@mail.google.com> <Y5UP+tnnxNgoi6A2@mail.google.com>
 <CAHVum0eOzd8MgP0FGObHWvqG_oPVoTmk_5gkEB0sAJK9JgCsFg@mail.gmail.com> <Y5ekQcJeoHd1i+Um@mail.google.com>
In-Reply-To: <Y5ekQcJeoHd1i+Um@mail.google.com>
From:   Vipin Sharma <vipinsh@google.com>
Date:   Mon, 12 Dec 2022 14:32:41 -0800
Message-ID: <CAHVum0fqOj9peepXdrAETTO3_qNX-B2qM41AnPdHMMgPNyrnKg@mail.gmail.com>
Subject: Re: [PATCH v2] scripts/tags.sh: choose which directories to exclude
 from being indexed
To:     Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Cc:     Masahiro Yamada <masahiroy@kernel.org>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Nicolas Schier <nicolas@fjasle.eu>,
        Jonathan Corbet <corbet@lwn.net>,
        Cristian Ciocaltea <cristian.ciocaltea@collabora.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kbuild@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Dec 12, 2022 at 1:59 PM Paulo Miguel Almeida
<paulo.miguel.almeida.rodenas@gmail.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@gmail.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.

>
> 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.

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.

> 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
> > >