* [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.
@ 2015-05-15 20:28 Dimitri John Ledkov
2015-05-15 20:43 ` Omar Sandoval
2015-05-21 8:19 ` Dimitri John Ledkov
0 siblings, 2 replies; 11+ messages in thread
From: Dimitri John Ledkov @ 2015-05-15 20:28 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.
2015-05-15 20:28 [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash Dimitri John Ledkov
@ 2015-05-15 20:43 ` Omar Sandoval
2015-05-16 8:27 ` Florian Gamböck
2015-05-21 8:19 ` Dimitri John Ledkov
1 sibling, 1 reply; 11+ messages in thread
From: Omar Sandoval @ 2015-05-15 20:43 UTC (permalink / raw)
To: Dimitri John Ledkov; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.
2015-05-15 20:43 ` Omar Sandoval
@ 2015-05-16 8:27 ` Florian Gamböck
2015-05-16 8:58 ` Omar Sandoval
2015-05-16 9:14 ` Markus Baertschi
0 siblings, 2 replies; 11+ messages in thread
From: Florian Gamböck @ 2015-05-16 8:27 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.
2015-05-16 8:27 ` Florian Gamböck
@ 2015-05-16 8:58 ` Omar Sandoval
2015-05-20 16:49 ` David Sterba
2015-05-16 9:14 ` Markus Baertschi
1 sibling, 1 reply; 11+ messages in thread
From: Omar Sandoval @ 2015-05-16 8:58 UTC (permalink / raw)
To: Florian Gamböck; +Cc: linux-btrfs
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.
2015-05-16 8:27 ` Florian Gamböck
2015-05-16 8:58 ` Omar Sandoval
@ 2015-05-16 9:14 ` Markus Baertschi
1 sibling, 0 replies; 11+ messages in thread
From: Markus Baertschi @ 2015-05-16 9:14 UTC (permalink / raw)
To: linux-btrfs
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)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.
2015-05-16 8:58 ` Omar Sandoval
@ 2015-05-20 16:49 ` David Sterba
0 siblings, 0 replies; 11+ messages in thread
From: David Sterba @ 2015-05-20 16:49 UTC (permalink / raw)
To: Omar Sandoval; +Cc: Florian Gamböck, linux-btrfs
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.
2015-05-15 20:28 [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash Dimitri John Ledkov
2015-05-15 20:43 ` Omar Sandoval
@ 2015-05-21 8:19 ` Dimitri John Ledkov
2015-05-21 11:56 ` David Sterba
1 sibling, 1 reply; 11+ messages in thread
From: Dimitri John Ledkov @ 2015-05-21 8:19 UTC (permalink / raw)
To: linux-btrfs
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash.
2015-05-21 8:19 ` Dimitri John Ledkov
@ 2015-05-21 11:56 ` David Sterba
2015-05-21 12:50 ` [PATCH] fsck.btrfs: Fix bashism and bad getopts processing Dimitri John Ledkov
0 siblings, 1 reply; 11+ messages in thread
From: David Sterba @ 2015-05-21 11:56 UTC (permalink / raw)
To: Dimitri John Ledkov; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] fsck.btrfs: Fix bashism and bad getopts processing
2015-05-21 11:56 ` David Sterba
@ 2015-05-21 12:50 ` Dimitri John Ledkov
2015-05-21 14:40 ` David Sterba
2015-05-25 12:28 ` David Sterba
0 siblings, 2 replies; 11+ messages in thread
From: Dimitri John Ledkov @ 2015-05-21 12:50 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] fsck.btrfs: Fix bashism and bad getopts processing
2015-05-21 12:50 ` [PATCH] fsck.btrfs: Fix bashism and bad getopts processing Dimitri John Ledkov
@ 2015-05-21 14:40 ` David Sterba
2015-05-25 12:28 ` David Sterba
1 sibling, 0 replies; 11+ messages in thread
From: David Sterba @ 2015-05-21 14:40 UTC (permalink / raw)
To: Dimitri John Ledkov; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] fsck.btrfs: Fix bashism and bad getopts processing
2015-05-21 12:50 ` [PATCH] fsck.btrfs: Fix bashism and bad getopts processing Dimitri John Ledkov
2015-05-21 14:40 ` David Sterba
@ 2015-05-25 12:28 ` David Sterba
1 sibling, 0 replies; 11+ messages in thread
From: David Sterba @ 2015-05-25 12:28 UTC (permalink / raw)
To: Dimitri John Ledkov; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-05-25 12:28 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-15 20:28 [PATCH] Fix bashism in fsck.btrfs for debian/ubuntu dash Dimitri John Ledkov
2015-05-15 20:43 ` Omar Sandoval
2015-05-16 8:27 ` Florian Gamböck
2015-05-16 8:58 ` Omar Sandoval
2015-05-20 16:49 ` David Sterba
2015-05-16 9:14 ` Markus Baertschi
2015-05-21 8:19 ` Dimitri John Ledkov
2015-05-21 11:56 ` David Sterba
2015-05-21 12:50 ` [PATCH] fsck.btrfs: Fix bashism and bad getopts processing Dimitri John Ledkov
2015-05-21 14:40 ` David Sterba
2015-05-25 12:28 ` David Sterba
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.