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:   Wed, 15 Jun 2022 17:14:06 +0800
From:   Leo Yan <leo.yan@linaro.org>
To:     Carsten Haitzler <carsten.haitzler@foss.arm.com>
Cc:     linux-kernel@vger.kernel.org, coresight@lists.linaro.org,
        suzuki.poulose@arm.com, mathieu.poirier@linaro.org,
        mike.leach@linaro.org, linux-perf-users@vger.kernel.org,
        acme@kernel.org
Subject: Re: [PATCH 2/3] perf test: Shell - only run .sh shell files to skip
 other files
Message-ID: <20220615091406.GB121786@leoy-ThinkPad-X240s>
References: <20220309122859.31487-1-carsten.haitzler@foss.arm.com>
 <20220309122859.31487-2-carsten.haitzler@foss.arm.com>
 <20220410022846.GB14326@leoy-ThinkPad-X240s>
 <9ab780aa-7e78-b0f9-21ed-cf30f41f8fab@foss.arm.com>
 <20220526101423.GB795802@leoy-ThinkPad-X240s>
 <ae8bdc2c-a444-74c1-70d5-c97d27b22ec5@foss.arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ae8bdc2c-a444-74c1-70d5-c97d27b22ec5@foss.arm.com>
Precedence: bulk

On Mon, Jun 13, 2022 at 02:08:30PM +0100, Carsten Haitzler wrote:

[...]

> > If the condition checking gets complex, seems to me it is reasonable to
> > use a static function (or a macro?) to encapsulate the logics.
> 
> Well normally my rule i s - if it gets re-used then do it, otherwise it just
> involves more indirection to follow. :) But regardless of that, given some
> other things you ask for that kind of makes this discussion moot as it
> requires much bigger wholesale changes to the test infra which will make
> these patches a lot more work. I'll get to that later in mails.

Your mentioned rule makes sense to me.

> > > But one catch... it really should be is_non_hidden_exe_shell_script() as
> > > it's checking that it's not a hidden file AND is a shell script. Or do I
> > > keep the hidden file test outside of the function in the if? If we're nit
> > > picking then I need to know exactly what you want here as your suggested
> > > name is actually incorrect.
> > 
> > I personally prefer to use the condition:
> > 
> >    if (is_exe_shell_script() && ent->d_name[0] != '.')
> >        do_something...
> > 
> > The reason is the function is_exe_shell_script() is more common and we
> > use it easily in wider scope.
> 
> As above - will probably have to redo a lot of the test infra involving the
> shell tests to handle some of your other requests, but if we don't go that
> way, I have got where you want to go and I can do this.

To be honest, I am not sure if this patch is related with refactoring
test infrastructure or not.  You could reconsider when you spin for next
patch set (as you said, might refactor test infra).

In case you still want to keep this patch as it is, it would be fine for
me and you could add my reviewed tag:

Reviewed-by: Leo Yan <leo.yan@linaro.org>

Thanks,
Leo