[PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.

[PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.

[Dimitri John Ledkov]

Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=784911
Signed-off-by: Dimitri John Ledkov <dimitri.j.ledkov@intel.com>
---
 fsck.btrfs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fsck.btrfs b/fsck.btrfs
index f056a7f..3a92804 100755
--- a/fsck.btrfs
+++ b/fsck.btrfs
@@ -31,7 +31,7 @@ if [ ! -e $DEV ]; then
 	echo "$0: $DEV does not exist"
 	exit 8
 fi
-if [ "$AUTO" == "false" ]; then
+if [ "false" = "$AUTO" ]; then
 	echo "If you wish to check the consistency of a BTRFS filesystem or"
 	echo "repair a damaged filesystem, see btrfs(8) subcommand 'check'."
 fi
-- 
2.1.4

Re: [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.

[Omar Sandoval]

On Fri, May 15, 2015 at 09:28:29PM +0100, Dimitri John Ledkov wrote:
> Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=784911
> Signed-off-by: Dimitri John Ledkov <dimitri.j.ledkov@intel.com>
> ---
>  fsck.btrfs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fsck.btrfs b/fsck.btrfs
> index f056a7f..3a92804 100755
> --- a/fsck.btrfs
> +++ b/fsck.btrfs
> @@ -31,7 +31,7 @@ if [ ! -e $DEV ]; then
>  	echo "$0: $DEV does not exist"
>  	exit 8
>  fi
> -if [ "$AUTO" == "false" ]; then
> +if [ "false" = "$AUTO" ]; then
>  	echo "If you wish to check the consistency of a BTRFS filesystem or"
>  	echo "repair a damaged filesystem, see btrfs(8) subcommand 'check'."
>  fi

I'm going to completely bikeshed here, but Yoda conditions are already
ugly in C, and completely pointless in Bash, where you can't ever
accidentally reassign a variable in a condition. Either way, I think:

if [ ! $AUTO ]; then

would be clearer anyways.

Thanks!
-- 
Omar

Re: [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.

[Florian Gamböck]

Am 15.05.2015 um 22:43 schrieb Omar Sandoval:
> I'm going to completely bikeshed here, but Yoda conditions are already
> ugly in C, and completely pointless in Bash, where you can't ever
> accidentally reassign a variable in a condition. Either way, I think:
>
> if [ ! $AUTO ]; then
>
> would be clearer anyways.

Ah, I'm sorry to disagree with you, but your code snippet would only 
work if $AUTO is *empty*, and I think, to be totally correct you'd have 
to use the -n or -z test.

To sum it up now, you'd have to replace "false" with an empty string in 
the beginning of the file and the zero-test in the end. So something 
like the following:

AUTO=
# ...
if [ -z "$AUTO" ]; then

Regards
--Flo

Re: [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.

[Omar Sandoval]

On Sat, May 16, 2015 at 10:27:11AM +0200, Florian Gamböck wrote:
> Am 15.05.2015 um 22:43 schrieb Omar Sandoval:
> >I'm going to completely bikeshed here, but Yoda conditions are already
> >ugly in C, and completely pointless in Bash, where you can't ever
> >accidentally reassign a variable in a condition. Either way, I think:
> >
> >if [ ! $AUTO ]; then
> >
> >would be clearer anyways.
> 
> Ah, I'm sorry to disagree with you, but your code snippet would only work if
> $AUTO is *empty*, and I think, to be totally correct you'd have to use the
> -n or -z test.
> 
> To sum it up now, you'd have to replace "false" with an empty string in the
> beginning of the file and the zero-test in the end. So something like the
> following:
> 
> AUTO=
> # ...
> if [ -z "$AUTO" ]; then
> 

Whoops, you're totally right, that was a typo. I meant

if ! $AUTO; then

-- 
Omar

Re: [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.

[David Sterba]

On Sat, May 16, 2015 at 01:58:28AM -0700, Omar Sandoval wrote:
> On Sat, May 16, 2015 at 10:27:11AM +0200, Florian Gamböck wrote:
> > Am 15.05.2015 um 22:43 schrieb Omar Sandoval:
> > >I'm going to completely bikeshed here, but Yoda conditions are already
> > >ugly in C, and completely pointless in Bash, where you can't ever
> > >accidentally reassign a variable in a condition. Either way, I think:
> > >
> > >if [ ! $AUTO ]; then
> > >
> > >would be clearer anyways.
> > 
> > Ah, I'm sorry to disagree with you, but your code snippet would only work if
> > $AUTO is *empty*, and I think, to be totally correct you'd have to use the
> > -n or -z test.
> > 
> > To sum it up now, you'd have to replace "false" with an empty string in the
> > beginning of the file and the zero-test in the end. So something like the
> > following:
> > 
> > AUTO=
> > # ...
> > if [ -z "$AUTO" ]; then
> > 
> 
> Whoops, you're totally right, that was a typo. I meant
> 
> if ! $AUTO; then

That's my preference as it was implemented originally. I did not pay
close attention to how it was implemented in "silence fake fsck" as far
as it worked.

Re: [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.

[Markus Baertschi]

If we just want to fix the bashism, then

        if [ "$AUTO" = "false" ]; then

is a good solution. No point to use a yoda condition.


But I like the style proposed by Florian best. It is closest to
traditional shell programming.

On Sat, May 16, 2015 at 10:27 AM, Florian Gamböck <ml@floga.de> wrote:
>
> AUTO=
> # ...
> if [ -z "$AUTO" ]; then

Markus

-- 
Markus Baertschi, Bas du Rossé 16, CH-1163, Etoy, Switzerland
markus@markus.org, +41 (79) 403 1186 (mobile)

Re: [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.

[Dimitri John Ledkov]

On 15 May 2015 at 21:28, Dimitri John Ledkov <dimitri.j.ledkov@intel.com> wrote:
> Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=784911
> Signed-off-by: Dimitri John Ledkov <dimitri.j.ledkov@intel.com>
> ---
>  fsck.btrfs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fsck.btrfs b/fsck.btrfs
> index f056a7f..3a92804 100755
> --- a/fsck.btrfs
> +++ b/fsck.btrfs
> @@ -31,7 +31,7 @@ if [ ! -e $DEV ]; then
>         echo "$0: $DEV does not exist"
>         exit 8
>  fi
> -if [ "$AUTO" == "false" ]; then
> +if [ "false" = "$AUTO" ]; then
>         echo "If you wish to check the consistency of a BTRFS filesystem or"
>         echo "repair a damaged filesystem, see btrfs(8) subcommand 'check'."
>  fi

There was a lot of discussion about this trivial patch, mostly because
it's so trivial and has many ways to fix.

I hope btrfs maintainers could implement & push any of the proposed
ways to resolve this bashism to rule them all =)

-- 
Regards,

Dimitri.

Re: [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.

[David Sterba]

On Thu, May 21, 2015 at 09:19:59AM +0100, Dimitri John Ledkov wrote:
> On 15 May 2015 at 21:28, Dimitri John Ledkov <dimitri.j.ledkov@intel.com> wrote:
> > Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=784911
> > Signed-off-by: Dimitri John Ledkov <dimitri.j.ledkov@intel.com>
> > ---
> >  fsck.btrfs | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fsck.btrfs b/fsck.btrfs
> > index f056a7f..3a92804 100755
> > --- a/fsck.btrfs
> > +++ b/fsck.btrfs
> > @@ -31,7 +31,7 @@ if [ ! -e $DEV ]; then
> >         echo "$0: $DEV does not exist"
> >         exit 8
> >  fi
> > -if [ "$AUTO" == "false" ]; then
> > +if [ "false" = "$AUTO" ]; then
> >         echo "If you wish to check the consistency of a BTRFS filesystem or"
> >         echo "repair a damaged filesystem, see btrfs(8) subcommand 'check'."
> >  fi
> 
> There was a lot of discussion about this trivial patch, mostly because
> it's so trivial and has many ways to fix.
> 
> I hope btrfs maintainers could implement & push any of the proposed
> ways to resolve this bashism to rule them all =)

If you're ok to do

if ! $AUTO...

with your signed-off, then I'll add the patch. I was not comfortable to
do it right away.

[PATCH] fsck.btrfs: Fix bashism and bad getopts processing

[Dimitri John Ledkov]

First fix == bashism, as that is not accepted by e.g. Debian/Ubuntu
dash.

Secondly shift OPTIND, such that last parameter is checked to exist.

Signed-off-by: Dimitri John Ledkov <dimitri.j.ledkov@intel.com>
---
 fsck.btrfs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fsck.btrfs b/fsck.btrfs
index f056a7f..20b070a 100755
--- a/fsck.btrfs
+++ b/fsck.btrfs
@@ -26,12 +26,13 @@ do
 	a|A|p|y)	AUTO=true;;
 	esac
 done
+shift $(($OPTIND -1))
 eval DEV=\${$#}
 if [ ! -e $DEV ]; then
 	echo "$0: $DEV does not exist"
 	exit 8
 fi
-if [ "$AUTO" == "false" ]; then
+if ! $AUTO; then
 	echo "If you wish to check the consistency of a BTRFS filesystem or"
 	echo "repair a damaged filesystem, see btrfs(8) subcommand 'check'."
 fi
-- 
2.1.4

Re: [PATCH] fsck.btrfs: Fix bashism and bad getopts processing

[David Sterba]

On Thu, May 21, 2015 at 01:50:55PM +0100, Dimitri John Ledkov wrote:
> First fix == bashism, as that is not accepted by e.g. Debian/Ubuntu
> dash.
> 
> Secondly shift OPTIND, such that last parameter is checked to exist.
> 
> Signed-off-by: Dimitri John Ledkov <dimitri.j.ledkov@intel.com>

Applied, thanks.

Re: [PATCH] fsck.btrfs: Fix bashism and bad getopts processing

[David Sterba]

On Thu, May 21, 2015 at 01:50:55PM +0100, Dimitri John Ledkov wrote:
> First fix == bashism, as that is not accepted by e.g. Debian/Ubuntu
> dash.
> 
> Secondly shift OPTIND, such that last parameter is checked to exist.
> 
> Signed-off-by: Dimitri John Ledkov <dimitri.j.ledkov@intel.com>
> ---
>  fsck.btrfs | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fsck.btrfs b/fsck.btrfs
> index f056a7f..20b070a 100755
> --- a/fsck.btrfs
> +++ b/fsck.btrfs
> @@ -26,12 +26,13 @@ do
>  	a|A|p|y)	AUTO=true;;
>  	esac
>  done
> +shift $(($OPTIND -1))

BTW, this line is missing in the fsck.xfs stub as well, you may want to
send the patch to xfsprogs.