| Message ID | 1431721709-5146-1-git-send-email-dimitri.j.ledkov@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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!
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 =)
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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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(-)