[PATCH v4] scripts/tags.sh: use `find` for $ALLSOURCE_ARCHS generation

[PATCH v4] scripts/tags.sh: use `find` for $ALLSOURCE_ARCHS generation

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 1207 bytes --]

Parsing `ls` is fragile at best and _will_ fail when $tree
contains spaces. Replace this with a find-generated string
and directly assign it to $ALLSOURCE_ARCHS; a subshell is
implied by $(), so `cd` doesn't affect the current working
directory.

Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>

 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/scripts/tags.sh b/scripts/tags.sh
index 78e546ff689c2d5f40..c08347fdeef12a7621 100755
--- a/scripts/tags.sh
+++ b/scripts/tags.sh
@@ -26,24 +26,15 @@ else
 fi
 
 # ignore userspace tools
 ignore="$ignore ( -path ${tree}tools ) -prune -o"
 
-# Find all available archs
-find_all_archs()
-{
-	ALLSOURCE_ARCHS=""
-	for arch in `ls ${tree}arch`; do
-		ALLSOURCE_ARCHS="${ALLSOURCE_ARCHS} "${arch##\/}
-	done
-}
-
 # Detect if ALLSOURCE_ARCHS is set. If not, we assume SRCARCH
 if [ "${ALLSOURCE_ARCHS}" = "" ]; then
 	ALLSOURCE_ARCHS=${SRCARCH}
 elif [ "${ALLSOURCE_ARCHS}" = "all" ]; then
-	find_all_archs
+	ALLSOURCE_ARCHS="$(find "${tree}arch/" -mindepth 1 -maxdepth 1 -type d -printf ' %f')"
 fi
 
 # find sources in arch/$ARCH
 find_arch_sources()
 {
-- 
2.17.0.rc1.35.g90bbd502d54fe92035.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4] scripts/tags.sh: use `find` for $ALLSOURCE_ARCHS generation

[Masahiro Yamada]

Hi.


The commit log is wrong.


2018-05-18 20:56 GMT+09:00 Joey Pabalinas <joeypabalinas@gmail.com>:
> Parsing `ls` is fragile at best and _will_ fail when $tree
> contains spaces.

This statement is wrong.

The cause of the problem is not using whatever command you use,
but missing quoting.
The following would work even  if $tree contains spaces:

    for arch in `ls "${tree}arch"`; do


BTW, what was your motivation of this patch?

Does ${tree} contain spaces?


If the file path contains spaces, the top Makefile terminates it earlier.

Makefile:128: *** main directory cannot contain spaces nor colons.  Stop.




> Replace this with a find-generated string
> and directly assign it to $ALLSOURCE_ARCHS; a subshell is
> implied by $(), so `cd` doesn't affect the current working
> directory.


This patch no longer uses `cd`



> Signed-off-by: Joey Pabalinas <joeypabalinas@gmail.com>
>
>  1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/scripts/tags.sh b/scripts/tags.sh
> index 78e546ff689c2d5f40..c08347fdeef12a7621 100755
> --- a/scripts/tags.sh
> +++ b/scripts/tags.sh
> @@ -26,24 +26,15 @@ else
>  fi
>
>  # ignore userspace tools
>  ignore="$ignore ( -path ${tree}tools ) -prune -o"
>
> -# Find all available archs
> -find_all_archs()
> -{
> -       ALLSOURCE_ARCHS=""
> -       for arch in `ls ${tree}arch`; do
> -               ALLSOURCE_ARCHS="${ALLSOURCE_ARCHS} "${arch##\/}
> -       done
> -}
> -
>  # Detect if ALLSOURCE_ARCHS is set. If not, we assume SRCARCH
>  if [ "${ALLSOURCE_ARCHS}" = "" ]; then
>         ALLSOURCE_ARCHS=${SRCARCH}
>  elif [ "${ALLSOURCE_ARCHS}" = "all" ]; then
> -       find_all_archs
> +       ALLSOURCE_ARCHS="$(find "${tree}arch/" -mindepth 1 -maxdepth 1 -type d -printf ' %f')"
>  fi
>
>  # find sources in arch/$ARCH
>  find_arch_sources()
>  {
> --
> 2.17.0.rc1.35.g90bbd502d54fe92035.dirty
>



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4] scripts/tags.sh: use `find` for $ALLSOURCE_ARCHS generation

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

On Tue, May 22, 2018 at 03:01:07PM +0900, Masahiro Yamada wrote:
> The commit log is wrong.
> 
> 
> 2018-05-18 20:56 GMT+09:00 Joey Pabalinas <joeypabalinas@gmail.com>:
> > Parsing `ls` is fragile at best and _will_ fail when $tree
> > contains spaces.
> 
> This statement is wrong.
> 
> The cause of the problem is not using whatever command you use,
> but missing quoting.
> The following would work even  if $tree contains spaces:
> 
>     for arch in `ls "${tree}arch"`; do

Ah, to be completely honest that case didn't even occur to me.

> BTW, what was your motivation of this patch?
> 
> Does ${tree} contain spaces?

> 
> If the file path contains spaces, the top Makefile terminates it earlier.
> 
> Makefile:128: *** main directory cannot contain spaces nor colons.  Stop.

It doesn't; I didn't realize the Makefile already had a guard against
spaces in paths. It was something I noticed when poking at something
else and I thought it might be something worth fixing.

But now I agree with you that this patch isn't really needed at all. I can
no longer think of a case where even the original

> for arch in `ls ${tree}arch`; do

would break since spaces are not allowed at all.

Well, on the bright side, the times you happen to be wrong are also the
times where you learn the most :)

-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4] scripts/tags.sh: use `find` for $ALLSOURCE_ARCHS generation

[Masahiro Yamada]

2018-05-22 17:21 GMT+09:00 Joey Pabalinas <joeypabalinas@gmail.com>:
> On Tue, May 22, 2018 at 03:01:07PM +0900, Masahiro Yamada wrote:
>> The commit log is wrong.
>>
>>
>> 2018-05-18 20:56 GMT+09:00 Joey Pabalinas <joeypabalinas@gmail.com>:
>> > Parsing `ls` is fragile at best and _will_ fail when $tree
>> > contains spaces.
>>
>> This statement is wrong.
>>
>> The cause of the problem is not using whatever command you use,
>> but missing quoting.
>> The following would work even  if $tree contains spaces:
>>
>>     for arch in `ls "${tree}arch"`; do
>
> Ah, to be completely honest that case didn't even occur to me.
>
>> BTW, what was your motivation of this patch?
>>
>> Does ${tree} contain spaces?
>
>>
>> If the file path contains spaces, the top Makefile terminates it earlier.
>>
>> Makefile:128: *** main directory cannot contain spaces nor colons.  Stop.
>
> It doesn't; I didn't realize the Makefile already had a guard against
> spaces in paths. It was something I noticed when poking at something
> else and I thought it might be something worth fixing.
>
> But now I agree with you that this patch isn't really needed at all. I can
> no longer think of a case where even the original
>
>> for arch in `ls ${tree}arch`; do
>
> would break since spaces are not allowed at all.
>
> Well, on the bright side, the times you happen to be wrong are also the
> times where you learn the most :)
>


I see some new motivations for this patch now.

  - The current code includes 'Kconfig' in ALLSOURCE_ARCHS,
    but it should not.

  - Simplify the code, removing the find_all_archs().


If you are motivated for v5,
how about this?

  - Remove the double-quotes from "${tree}arch/"
    (Because the rest parts of this script do not have double-quoting,
     I do not see point in adding it in this line only.)

  - Update the git-log to not mention the hypothetical space things.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v4] scripts/tags.sh: use `find` for $ALLSOURCE_ARCHS generation

[Joey Pabalinas]

[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]

On Tue, May 22, 2018 at 05:41:48PM +0900, Masahiro Yamada wrote:
> I see some new motivations for this patch now.
> 
>   - The current code includes 'Kconfig' in ALLSOURCE_ARCHS,
>     but it should not.
> 
>   - Simplify the code, removing the find_all_archs().
> 
> 
> If you are motivated for v5,
> how about this?
> 
>   - Remove the double-quotes from "${tree}arch/"
>     (Because the rest parts of this script do not have double-quoting,
>      I do not see point in adding it in this line only.)
> 
>   - Update the git-log to not mention the hypothetical space things.

Alright, that sounds reasonable. What do you think of:

> ALLSOURCE_ARCHS=$(find ${tree}arch/ -mindepth 1 -maxdepth 1 -type d -printf '%f ')

The double quotes surrounding the arguments have been removed to be more
consistent with the other find commands in the script.

The ' %f' was changed to '%f ' so that you don't get that weird leading
space; I couldn't think of a way this could break anything but I wanted
to be 100% sure instead of needing to make a v6, heh.

The commit message will definitely be fixed as well.

-- 
Cheers,
Joey Pabalinas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4] scripts/tags.sh: use `find` for $ALLSOURCE_ARCHS generation

[Masahiro Yamada]

2018-05-22 18:06 GMT+09:00 Joey Pabalinas <joeypabalinas@gmail.com>:
> On Tue, May 22, 2018 at 05:41:48PM +0900, Masahiro Yamada wrote:
>> I see some new motivations for this patch now.
>>
>>   - The current code includes 'Kconfig' in ALLSOURCE_ARCHS,
>>     but it should not.
>>
>>   - Simplify the code, removing the find_all_archs().
>>
>>
>> If you are motivated for v5,
>> how about this?
>>
>>   - Remove the double-quotes from "${tree}arch/"
>>     (Because the rest parts of this script do not have double-quoting,
>>      I do not see point in adding it in this line only.)
>>
>>   - Update the git-log to not mention the hypothetical space things.
>
> Alright, that sounds reasonable. What do you think of:
>
>> ALLSOURCE_ARCHS=$(find ${tree}arch/ -mindepth 1 -maxdepth 1 -type d -printf '%f ')
>
> The double quotes surrounding the arguments have been removed to be more
> consistent with the other find commands in the script.
>
> The ' %f' was changed to '%f ' so that you don't get that weird leading
> space; I couldn't think of a way this could break anything but I wanted
> to be 100% sure instead of needing to make a v6, heh.


Fine with me.







-- 
Best Regards
Masahiro Yamada