create-pull-request: cd to relative directory

create-pull-request -d path creates empty patches if directory is specified as a path, i.e. ./bitbake or ./bitbake/ or full path. It behaves expected way only if script is run with -d bitbake, i.e. relative dir name doesn't contain '\'.

Fixed this unwanted behaviour by changing directory and running git format-patch in it with --relative, without specifying relative path as a parameter.

Signed-off-by Ed Bartosh <[email protected]>

---
 scripts/create-pull-request | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

scripts/create-pull-request +0 -0

index 216edfd..7eac618 100755

scripts/create-pull-request +3 -1

+       extraopts="--relative"
 fi
 
 # Generate the patches and cover letter
 git format-patch $extraopts -M40 --subject-prefix="$PREFIX" -n -o $ODIR --thread=shallow --cover-letter $RELATIVE_TO..$COMMIT_ID > /dev/null
 
+[ -n "$RELDIR" ] && popd
+
 # Customize the cover letter
 CL="$ODIR/0000-cover-letter.patch"
 PM="$ODIR/pull-msg"
--

2.1.4

--
_______________________________________________
Openembedded-core mailing list
[email protected]
http://lists.openembedded.org/mailman/listinfo/openembedded-core

On Wed, Aug 5, 2015 at 2:01 AM, Ed Bartosh <[email protected]>

Quoted 21 lines

create-pull-request -d path creates empty patches if directory is specified as a path, i.e. ./bitbake or ./bitbake/ or full path. It behaves expected way only if script is run with -d bitbake, i.e. relative dir name doesn't contain '\'.

Fixed this unwanted behaviour by changing directory and running git format-patch in it with --relative, without specifying relative path as a parameter.

Signed-off-by Ed Bartosh <[email protected]>

---

 scripts/create-pull-request | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

scripts/create-pull-request +0 -0

index 216edfd..7eac618 100755
@@ -177,12 +177,15 @@ mkdir $ODIR
 

scripts/create-pull-request mkdir $ODIR

Quoted 6 lines

scripts/create-pull-request +1 -1

 if [ -n "$RELDIR" ]; then
 
        ODIR=$(realpath $ODIR)
 
-       extraopts="--relative=$RELDIR"
+       pushd $RELDIR

can we avoid using pushd so it works with non bash shells too ?

Quoted 1 line · On Wednesday 05 August 2015 20:33:48 Khem Raj wrote:

On Wed, Aug 5, 2015 at 2:01 AM, Ed Bartosh <[email protected]>

Quoted 25 lines · wrote:

Quoted 25 lines

create-pull-request -d path creates empty patches if directory is specified as a path, i.e. ./bitbake or ./bitbake/ or full path. It behaves expected way only if script is run with -d bitbake, i.e. relative dir name doesn't contain '\'.

Fixed this unwanted behaviour by changing directory and running git format-patch in it with --relative, without specifying relative path as a parameter.

Signed-off-by Ed Bartosh <[email protected]>

---

 scripts/create-pull-request | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

scripts/create-pull-request +0 -0

index 216edfd..7eac618 100755
@@ -177,12 +177,15 @@ mkdir $ODIR
 
 if [ -n "$RELDIR" ]; then
 
        ODIR=$(realpath $ODIR)
 

Quoted 4 lines

scripts/create-pull-request mkdir $ODIR · line 180

Quoted 2 lines

scripts/create-pull-request +1 -1

-       extraopts="--relative=$RELDIR"
+       pushd $RELDIR

can we avoid using pushd so it works with non bash shells too ?

Should be possible, but it's worth mentioning that this script already starts with #!/bin/bash.

Quoted 1 line · On Wednesday 05 August 2015 20:33:48 Khem Raj wrote:

On Wed, Aug 5, 2015 at 2:01 AM, Ed Bartosh <[email protected]>

Quoted 25 lines · wrote:

Quoted 25 lines

create-pull-request -d path creates empty patches if directory is specified as a path, i.e. ./bitbake or ./bitbake/ or full path. It behaves expected way only if script is run with -d bitbake, i.e. relative dir name doesn't contain '\'.

Fixed this unwanted behaviour by changing directory and running git format-patch in it with --relative, without specifying relative path as a parameter.

Signed-off-by Ed Bartosh <[email protected]>

---

scripts/create-pull-request | 5 ++++-

1 file changed, 4 insertions(+), 1 deletion(-)

scripts/create-pull-request +0 -0

index 216edfd..7eac618 100755
@@ -177,12 +177,15 @@ mkdir $ODIR
 

if [ -n "$RELDIR" ]; then

       ODIR=$(realpath $ODIR)

Quoted 4 lines

scripts/create-pull-request mkdir $ODIR · line 180

Quoted 2 lines

scripts/create-pull-request +1 -1

-       extraopts="--relative=$RELDIR"
+       pushd $RELDIR

can we avoid using pushd so it works with non bash shells too ?

Should be possible, but it's worth mentioning that this script already starts with #!/bin/bash.

Cheers,
Paul

--

Paul Eggleton
Intel Open Source Technology Centre

Quoted 36 lines

On Aug 6, 2015, at 1:38 AM, Paul Eggleton <[email protected]> wrote:>

Quoted 1 line · On Wednesday 05 August 2015 20:33:48 Khem Raj wrote:

On Wed, Aug 5, 2015 at 2:01 AM, Ed Bartosh <[email protected]>

Quoted 29 lines · wrote:

Quoted 25 lines

create-pull-request -d path creates empty patches if directory is specified as a path, i.e. ./bitbake or ./bitbake/ or full path. It behaves expected way only if script is run with -d bitbake, i.e. relative dir name doesn't contain '\'.

Fixed this unwanted behaviour by changing directory and running git format-patch in it with --relative, without specifying relative path as a parameter.

Signed-off-by Ed Bartosh <[email protected]>

---

scripts/create-pull-request | 5 ++++-

1 file changed, 4 insertions(+), 1 deletion(-)

scripts/create-pull-request +0 -0

index 216edfd..7eac618 100755
@@ -177,12 +177,15 @@ mkdir $ODIR
 

if [ -n "$RELDIR" ]; then

       ODIR=$(realpath $ODIR)

scripts/create-pull-request mkdir $ODIR · line 180

Quoted 2 lines

scripts/create-pull-request +1 -1

-       extraopts="--relative=$RELDIR"
+       pushd $RELDIR

can we avoid using pushd so it works with non bash shells too ?

Quoted 2 lines

Should be possible, but it's worth mentioning that this script already starts with #!/bin/bash.

Does it have to be ?

Quoted 36 lines · On Thursday 06 August 2015 08:05:40 Khem Raj wrote:

Quoted 36 lines

On Aug 6, 2015, at 1:38 AM, Paul Eggleton <[email protected]> wrote:>

Quoted 1 line · On Wednesday 05 August 2015 20:33:48 Khem Raj wrote:

On Wed, Aug 5, 2015 at 2:01 AM, Ed Bartosh <[email protected]>

Quoted 29 lines · wrote:

Quoted 25 lines

create-pull-request -d path creates empty patches if directory is specified as a path, i.e. ./bitbake or ./bitbake/ or full path. It behaves expected way only if script is run with -d bitbake, i.e. relative dir name doesn't contain '\'.

Fixed this unwanted behaviour by changing directory and running git format-patch in it with --relative, without specifying relative path as a parameter.

Signed-off-by Ed Bartosh <[email protected]>

---

scripts/create-pull-request | 5 ++++-

1 file changed, 4 insertions(+), 1 deletion(-)

scripts/create-pull-request +0 -0

index 216edfd..7eac618 100755
@@ -177,12 +177,15 @@ mkdir $ODIR
 

if [ -n "$RELDIR" ]; then

      ODIR=$(realpath $ODIR)

scripts/create-pull-request mkdir $ODIR · line 180

Quoted 2 lines

scripts/create-pull-request +1 -1

-       extraopts="--relative=$RELDIR"
+       pushd $RELDIR

can we avoid using pushd so it works with non bash shells too ?

Quoted 4 lines

Quoted 2 lines

Should be possible, but it's worth mentioning that this script already starts with #!/bin/bash.

Does it have to be ?

Are you volunteering to fix any other bashisms in the script? If so no ;)

Cheers,
Paul

--

Paul Eggleton
Intel Open Source Technology Centre

Made create-pull-request POSIX compatible:
- Replaced /bin/bash -> /bin/sh in shebang.
- Replaced usage of pushd/popd with generic shell commands.
- Tested on zsh and dash.

Signed-off-by Ed Bartosh <[email protected]>

---
 scripts/create-pull-request | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

scripts/create-pull-request +4 -4

index be49379..19ba588 100755
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/bin/sh
 #
 # Copyright (c) 2010-2013,  Intel Corporation.
 # All Rights Reserved

@@ -177,14 +177,15 @@ mkdir $ODIR
 
 if [ -n "$RELDIR" ]; then
        ODIR=$(realpath $ODIR)
-       pushd $RELDIR > /dev/null
+       pdir=$(pwd)
+       cd $RELDIR
        extraopts="--relative"
 fi
 
 # Generate the patches and cover letter
 git format-patch $extraopts -M40 --subject-prefix="$PREFIX" -n -o $ODIR --thread=shallow --cover-letter $RELATIVE_TO..$COMMIT_ID > /dev/null
 
-[ -n "$RELDIR" ] && popd > /dev/null
+[ -n "$RELDIR" ] && cd $pdir
 
 # Customize the cover letter
 CL="$ODIR/0000-cover-letter.patch"
--

2.1.4

--
_______________________________________________
Openembedded-core mailing list
[email protected]
http://lists.openembedded.org/mailman/listinfo/openembedded-core

Made create-pull-request POSIX compatible:
- Replaced /bin/bash -> /bin/sh in shebang.
- Replaced usage of pushd/popd with generic shell commands.
- Tested on zsh and dash.

This reminded me of the problems I've seen with various oe-core shell scripts:
they are missing systematic error handling.

IMO using bash and 'set -euxo pipefail' are a good approach to catch errors early in shell scripts. Manually checking for $? is error prone and things like pipes can hide them. Using undefined variables is another thing which should fail early before things go horribly wrong. And in build automation I'd rather see output of -x since things can fail in mysterious ways. And then there's quoting....

One useful guide is to use shellcheck as static analysis tool for shell scripts.

I guess internally bitbake uses at least set -e.

Comments?

Quoted 1 line · On Wed, Aug 12, 2015 at 2:49 AM, <[email protected]> wrote:

This reminded me of the problems I've seen with various oe-core shell

scripts:

Quoted 3 lines

they are missing systematic error handling.

IMO using bash and 'set -euxo pipefail' are a good approach to catch

errors

Quoted 1 line

early in shell scripts. Manually checking for $? is error prone and

things

Quoted 2 lines

like pipes can hide them. Using undefined variables is another thing which should fail early before things go horribly wrong. And in build

automation

Quoted 4 lines

I'd rather see output of -x since things can fail in mysterious ways. And then there's quoting....

One useful guide is to use shellcheck as static analysis tool for shell

scripts.

Quoted 1 line

Quoted 3 lines

I guess internally bitbake uses at least set -e.

Comments?

all these are good suggestions, I would recommend to open bugs for all of these some of them might need
to cite the scripts which need fixing.

Quoted 18 lines · Khem Raj

Quoted 1 line · On Wed, Aug 12, 2015 at 2:49 AM, <[email protected]> wrote:

This reminded me of the problems I've seen with various oe-core shell

scripts:

Quoted 3 lines

they are missing systematic error handling.

IMO using bash and 'set -euxo pipefail' are a good approach to catch

errors

Quoted 1 line

early in shell scripts. Manually checking for $? is error prone and

things

Quoted 2 lines

like pipes can hide them. Using undefined variables is another thing which should fail early before things go horribly wrong. And in build

automation

Quoted 4 lines

I'd rather see output of -x since things can fail in mysterious ways. And then there's quoting....

One useful guide is to use shellcheck as static analysis tool for shell

scripts.

Quoted 1 line

Quoted 7 lines

Quoted 3 lines

I guess internally bitbake uses at least set -e.

Comments?

all these are good suggestions, I would recommend to open bugs for all of these some of them might need
to cite the scripts which need fixing.

Agreed, it's important to check the behavior of shell scripts is kosher and standards-compliant. shellcheck is extremely useful, I recommend it as well, but combined with the vim syntastic plugin, so every time you save the file all the errors and warnings are identified :) Makes it harder to ignore or forget about.

That reminds me, there's a shell portability issue / standards-complaince issue that's not identified by shellcheck. Typically, to negate a bracket expression in a regular expression, one uses ^, e.g. [^a-z] is everything that isn't in the range a to z, but that's not the case in shell, e.g. at a prompt or in a ${foo#<pattern>}:

"[...]a bracket expression as in XBD *RE Bracket Expression* , except
that the <exclamation-mark> character ( '!' ) shall replace the <circumflex> character ( '^' ) in its role in a non-matching list in the regular expression notation"

So in shell, you'd want [!a-z] rather than [^a-z]. Nearly all shells handle both, but the behavior of the latter is actually unspecified according to the standard:

"A bracket expression starting with an unquoted <circumflex> character
produces unspecified results."

I recently got bitten by this with one of my shell scripts on a system running dash.

Quoted 2 lines · Christopher Larson

That reminds me, there's a shell portability issue / standards-complaince issue that's not identified by shellcheck. Typically, to negate a

bracket

Quoted 2 lines

expression in a regular expression, one uses ^, e.g. [^a-z] is everything that isn't in the range a to z, but that's not the case in shell, e.g.

at a

Quoted 8 lines

prompt or in a ${foo#<pattern>}:

"[...]a bracket expression as in XBD *RE Bracket Expression* , except
that the <exclamation-mark> character ( '!' ) shall replace the <circumflex> character ( '^' ) in its role in a non-matching list in the regular expression notation"

So in shell, you'd want [!a-z] rather than [^a-z]. Nearly all shells

handle

Quoted 3 lines

both, but the behavior of the latter is actually unspecified according to the standard:

Quoted 1 line

"A bracket expression starting with an unquoted <circumflex>

character

Quoted 4 lines

produces unspecified results."

I recently got bitten by this with one of my shell scripts on a system running dash.

Does checkbashisms warn about this?

Quoted 27 lines · On Wed, Aug 12, 2015 at 11:27 PM, <[email protected]> wrote:

Quoted 1 line · Christopher Larson

That reminds me, there's a shell portability issue /

standards-complaince

Quoted 1 line

issue that's not identified by shellcheck. Typically, to negate a

bracket

Quoted 1 line

expression in a regular expression, one uses ^, e.g. [^a-z] is

everything

Quoted 1 line

that isn't in the range a to z, but that's not the case in shell, e.g.

at a

Quoted 3 lines

prompt or in a ${foo#<pattern>}:

"[...]a bracket expression as in XBD *RE Bracket Expression* ,

except

Quoted 5 lines

that the <exclamation-mark> character ( '!' ) shall replace the <circumflex> character ( '^' ) in its role in a non-matching list in the regular expression notation"

So in shell, you'd want [!a-z] rather than [^a-z]. Nearly all shells

handle

Quoted 1 line

both, but the behavior of the latter is actually unspecified according

to

Quoted 3 lines

the standard:

"A bracket expression starting with an unquoted <circumflex>

character

Quoted 2 lines

produces unspecified results."

Quoted 4 lines

Quoted 2 lines

I recently got bitten by this with one of my shell scripts on a system running dash.

Does checkbashisms warn about this?

Finally got around to checking, and yeah, checkbashisms does spot this one, it's just shellcheck that doesn't (yet, opened a bug upstream).

Update: checkbashisms and shellcheck both check for this now.

Quoted 1 line · Christopher Larson

Update: checkbashisms and shellcheck both check for this now.

This is great! How about formulating that oe-core shell scripts should pass shellcheck and checkbashisms checks without warnings?