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;
Date:   Tue, 13 Dec 2022 10:59:29 +1300
From:   Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
To:     Vipin Sharma <vipinsh@google.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
Subject: Re: [PATCH v2] scripts/tags.sh: choose which directories to exclude
 from being indexed
Message-ID: <Y5ekQcJeoHd1i+Um@mail.google.com>
References: <Y5T66yWNVAZNIaJ0@mail.google.com>
 <Y5UP+tnnxNgoi6A2@mail.google.com>
 <CAHVum0eOzd8MgP0FGObHWvqG_oPVoTmk_5gkEB0sAJK9JgCsFg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHVum0eOzd8MgP0FGObHWvqG_oPVoTmk_5gkEB0sAJK9JgCsFg@mail.gmail.com>
Precedence: bulk

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

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

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