Re: [PATCH v3 02/25] mbedtls: Add script to update MbedTLS subtree
On Wed, Jun 05, 2024 at 10:11:04AM +0300, Ilias Apalodimas wrote:
> On Tue, 4 Jun 2024 at 23:10, Andy Shevchenko
> wrote:
> > On Fri, May 31, 2024 at 09:32:38AM +0300, Ilias Apalodimas wrote:
> > > On Tue, 28 May 2024 at 17:10, Raymond Mao wrote:
...
> > > > +if ! git remote get-url mbedtls_upstream 2>/dev/null
> > >
> > > if [ -z "$(git remote get-url rigin 2>/dev/null)" ]; then
> >
> > Why? I mean why do we need an additional `test` call? Above can be
> > transformed
> > to `foo && {}` notation to get rid of if completely.
>
> That's the usual syntax we have in other scripts
Maybe, but it doesn't mean that "usual" syntax is not suboptimal or well
portable.
> > > > +then
> > > > +echo "Warning: Script automatically adds new git remote via:"
> > > > +echo "git remote add mbedtls_upstream \\"
> > > > +echo "https://github.com/Mbed-TLS/mbedtls.git;
> > > > +git remote add mbedtls_upstream \
> > > > +https://github.com/Mbed-TLS/mbedtls.git
> > > > +fi
> > > > +git fetch mbedtls_upstream master
> > > > +}
...
> > > > +if [ "$1" = "pull" ]
> > >
> > > "$1" == 'pull'
> >
> > Why? Isn't this bashism?
>
> You don't need variable expansion here, so I don't see why you need
> "". Unless you mean the ==, I am fine leaving that to =
The latter one. Since the script is shebanged with sh, it should be compatible
with any shell. Shell is hard to learn programming language, more than 98%
people can't write shell scripts properly, unfortunately. But this is our
legacy...
> > > Also on string literals, you don't need "", 'pull' is enough
That's okay.
...
> > > > +elif [ "$1" = "pick" ]
> > > move then 'then' one line up and add a ;
> >
> > > == 'pick'
> >
> > Ditto.
> >
> > > > +then
> > > > +remote_add_and_fetch
> > > > +git cherry-pick -x --strategy=subtree \
> > > > +-Xsubtree=lib/mbedtls/external/mbedtls/ "$2"
> > > > +else
> > > > +echo "usage: $0 "
> > > > +echo " pull or pick"
> > > > +echo " release tag [pull] or commit id [pick]"
> > > > +fi
> >
> > Sheel should be written as much as portable and less verbose.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 02/25] mbedtls: Add script to update MbedTLS subtree
On Tue, 4 Jun 2024 at 23:10, Andy Shevchenko
wrote:
>
> On Fri, May 31, 2024 at 09:32:38AM +0300, Ilias Apalodimas wrote:
> > On Tue, 28 May 2024 at 17:10, Raymond Mao wrote:
> > >
> > > lib/mbedtls/update-mbedtls-subtree.sh is a wrapper of git subtree
> > > commands.
> > > Usage from U-Boot top directory, run:
>
> ...
>
> > > +if ! git remote get-url mbedtls_upstream 2>/dev/null
> >
> > if [ -z "$(git remote get-url rigin 2>/dev/null)" ]; then
>
> Why? I mean why do we need an additional `test` call? Above can be transformed
> to `foo && {}` notation to get rid of if completely.
That's the usual syntax we have in other scripts
>
> > > +then
> > > +echo "Warning: Script automatically adds new git remote via:"
> > > +echo "git remote add mbedtls_upstream \\"
> > > +echo "https://github.com/Mbed-TLS/mbedtls.git;
> > > +git remote add mbedtls_upstream \
> > > +https://github.com/Mbed-TLS/mbedtls.git
> > > +fi
> > > +git fetch mbedtls_upstream master
> > > +}
>
> ...
>
> > > +if [ "$1" = "pull" ]
> >
> > "$1" == 'pull'
>
> Why? Isn't this bashism?
You don't need variable expansion here, so I don't see why you need
"". Unless you mean the ==, I am fine leaving that to =
>
> > Also on string literals, you don't need "", 'pull' is enough
> >
> > > +then
> > > +remote_add_and_fetch
> > > +git subtree pull --prefix lib/mbedtls/external/mbedtls
> > > mbedtls_upstream \
> > > +"$2" --squash -m "${merge_commit_msg}"
> > > +elif [ "$1" = "pick" ]
> > move then 'then' one line up and add a ;
>
> > == 'pick'
>
> Ditto.
>
> > > +then
> > > +remote_add_and_fetch
> > > +git cherry-pick -x --strategy=subtree \
> > > +-Xsubtree=lib/mbedtls/external/mbedtls/ "$2"
> > > +else
> > > +echo "usage: $0 "
> > > +echo " pull or pick"
> > > +echo " release tag [pull] or commit id [pick]"
> > > +fi
>
> Sheel should be written as much as portable and less verbose.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Re: [PATCH v3 02/25] mbedtls: Add script to update MbedTLS subtree
On Fri, May 31, 2024 at 09:32:38AM +0300, Ilias Apalodimas wrote:
> On Tue, 28 May 2024 at 17:10, Raymond Mao wrote:
> >
> > lib/mbedtls/update-mbedtls-subtree.sh is a wrapper of git subtree
> > commands.
> > Usage from U-Boot top directory, run:
...
> > +if ! git remote get-url mbedtls_upstream 2>/dev/null
>
> if [ -z "$(git remote get-url rigin 2>/dev/null)" ]; then
Why? I mean why do we need an additional `test` call? Above can be transformed
to `foo && {}` notation to get rid of if completely.
> > +then
> > +echo "Warning: Script automatically adds new git remote via:"
> > +echo "git remote add mbedtls_upstream \\"
> > +echo "https://github.com/Mbed-TLS/mbedtls.git;
> > +git remote add mbedtls_upstream \
> > +https://github.com/Mbed-TLS/mbedtls.git
> > +fi
> > +git fetch mbedtls_upstream master
> > +}
...
> > +if [ "$1" = "pull" ]
>
> "$1" == 'pull'
Why? Isn't this bashism?
> Also on string literals, you don't need "", 'pull' is enough
>
> > +then
> > +remote_add_and_fetch
> > +git subtree pull --prefix lib/mbedtls/external/mbedtls
> > mbedtls_upstream \
> > +"$2" --squash -m "${merge_commit_msg}"
> > +elif [ "$1" = "pick" ]
> move then 'then' one line up and add a ;
> == 'pick'
Ditto.
> > +then
> > +remote_add_and_fetch
> > +git cherry-pick -x --strategy=subtree \
> > +-Xsubtree=lib/mbedtls/external/mbedtls/ "$2"
> > +else
> > +echo "usage: $0 "
> > +echo " pull or pick"
> > +echo " release tag [pull] or commit id [pick]"
> > +fi
Sheel should be written as much as portable and less verbose.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 02/25] mbedtls: Add script to update MbedTLS subtree
On Tue, 28 May 2024 at 17:10, Raymond Mao wrote:
>
> lib/mbedtls/update-mbedtls-subtree.sh is a wrapper of git subtree
> commands.
> Usage from U-Boot top directory, run:
>
> $ ./lib/mbedtls/update-mbedtls-subtree.sh pull
> $ ./lib/mbedtls/update-mbedtls-subtree.sh pick
>
> Signed-off-by: Raymond Mao
> ---
> Changes in v2
> - Initial patch.
> Changes in v3
> - None.
>
> lib/mbedtls/update-mbedtls-subtree.sh | 50 +++
> 1 file changed, 50 insertions(+)
> create mode 100755 lib/mbedtls/update-mbedtls-subtree.sh
>
> diff --git a/lib/mbedtls/update-mbedtls-subtree.sh
> b/lib/mbedtls/update-mbedtls-subtree.sh
> new file mode 100755
> index 000..f208e54a5af
> --- /dev/null
> +++ b/lib/mbedtls/update-mbedtls-subtree.sh
> @@ -0,0 +1,50 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# Copyright 2024 Linaro Ltd.
> +#
> +# Usage: from the top level U-Boot source tree, run:
> +# $ ./lib/mbedtls/update-mbedtls-subtree.sh pull
> +# $ ./lib/mbedtls/update-mbedtls-subtree.sh pick
LWIP will add similar machinery, we want to merge these in a single script.
So, please move this to tools/ and adjust it accordingly as a first step
> +#
> +# The script will pull changes from MbedTLS repo into U-Boot
> +# as a subtree located as /lib/mbedtls/external/mbedtls
> sub-directory.
> +# It will automatically create a squash/merge commit listing the commits
> +# imported.
> +
> +set -e
> +
> +merge_commit_msg=$(cat << EOF
> +Subtree merge tag '$2' of MbedTLS repo [1] into lib/mbedtls/external/mbedtls
> +
> +[1] https://github.com/Mbed-TLS/mbedtls.git
> +EOF
> +)
> +
> +remote_add_and_fetch() {
> +if ! git remote get-url mbedtls_upstream 2>/dev/null
if [ -z "$(git remote get-url rigin 2>/dev/null)" ]; then
> +then
> +echo "Warning: Script automatically adds new git remote via:"
> +echo "git remote add mbedtls_upstream \\"
> +echo "https://github.com/Mbed-TLS/mbedtls.git;
> +git remote add mbedtls_upstream \
> +https://github.com/Mbed-TLS/mbedtls.git
> +fi
> +git fetch mbedtls_upstream master
> +}
> +
> +if [ "$1" = "pull" ]
"$1" == 'pull'
Also on string literals, you don't need "", 'pull' is enough
> +then
> +remote_add_and_fetch
> +git subtree pull --prefix lib/mbedtls/external/mbedtls mbedtls_upstream \
> +"$2" --squash -m "${merge_commit_msg}"
> +elif [ "$1" = "pick" ]
move then 'then' one line up and add a ;
== 'pick'
> +then
> +remote_add_and_fetch
> +git cherry-pick -x --strategy=subtree \
> +-Xsubtree=lib/mbedtls/external/mbedtls/ "$2"
> +else
> +echo "usage: $0 "
> +echo " pull or pick"
> +echo " release tag [pull] or commit id [pick]"
> +fi
> --
> 2.25.1
>
Cheers
/Ilias
