fix umask handling of symbolic modes

This fixes the handling of symbolic umask values. It also removes two
invocations of `perl` and all but two `math` commands.

Fixes #738

CHANGELOG.md

@@ -12,6 +12,7 @@
 - Empty CDPATH elements are now equivalent to "." (#2106).
 - The `read` command now accepts simple strings for the prompt rather than fish script via the new `-P` and `--prompt-str` flags (#802).
 - `type` now no longer requires `which`, which means fish no longer uses it anywhere. Packagers should remove the dependency (#3912).
+- Using symbolic permissions with the `umask` command now works (#738).
 
 ---
 

share/functions/umask.fish

@@ -1,117 +1,129 @@
+# Support the usual (i.e., bash compatible) `umask` UI. This reports or modifies the magic global
+# `umask` variable which is monitored by the fish process.
 
-function __fish_umask_parse -d "Internal umask function"
+# This table is indexed by the base umask value to be modified. Each digit represents the new umask
-    # Test if already a valid octal mask, and pad it with zeros
+# value when the permissions to add are applied to the base umask value.
-    if echo $argv | __fish_sgrep -E '^0?[0-7]{1,3}$' >/dev/null
+set __fish_umask_add_table 0101010 2002200 2103210 4440000 4541010 6442200 6543210
-        set -l char_count (echo $argv| wc -c)
+
-        for i in (seq (math 5 - $char_count))
+function __fish_umask_add
-            set argv 0$argv
+    set -l mask_digit $argv[1]
-        end
+    set -l to_add $argv[2]
-        echo $argv
+
-    else
+    set -l mask_table 0000000
-        # Test if argument really is a valid symbolic mask
+    if test $mask_digit -gt 0
-        if not echo $argv | __fish_sgrep -E '^(((u|g|o|a|)(=|\+|-)|)(r|w|x)*)(,(((u|g|o|a|)(=|\+|-)|)(r|w|x)*))*$' >/dev/null
+        set mask_table $__fish_umask_add_table[$mask_digit]
+    end
+    set -l new_vals (string split '' $mask_table)
+    echo $new_vals[$to_add]
+end
+
+# This table is indexed by the base umask value to be modified. Each digit represents the new umask
+# value when the permissions to remove are applied to the base umask value.
+set __fish_umask_remove_table 1335577 3236767 3337777 5674567 5775577 7676767 7777777
+
+function __fish_umask_remove
+    set -l mask_digit $argv[1]
+    set -l to_remove $argv[2]
+
+    set -l mask_table 1234567
+    if test $mask_digit -gt 0
+        set mask_table $__fish_umask_remove_table[$mask_digit]
+    end
+    set -l new_vals (string split '' $mask_table)
+    echo $new_vals[$to_remove]
+end
+
+# This returns the mask corresponding to allowing the permissions to allow. In other words it
+# returns the inverse of the mask passed in.
+set __fish_umask_set_table 6 5 4 3 2 1 0
+function __fish_umask_set
+    set -l to_set $argv[1]
+    if test $to_set -eq 0
+        return 7
+    end
+    echo $__fish_umask_set_table[$to_set]
+end
+
+# Given a umask string, possibly in symbolic mode, return an octal value with leading zeros.
+# This function expects to be called with a single value.
+function __fish_umask_parse
+    # Test if already a valid octal mask. If so pad it with zeros and return it.
+    # Note that umask values are always base 8 so they don't require a leading zero.
+    if string match -qr '^0?[0-7]{1,3}$' -- $argv
+        string sub -s -4 0000$argv
+        return 0
+    end
+
+    # Test if argument is a valid symbolic mask. Note that the basic pattern allows one illegal
+    # pattern: who and perms without a mode such as "urw". We test for that below after using the
+    # pattern to split the rights then testing for that invalid combination.
+    set -l basic_pattern '([ugoa]*)([=+-]?)([rwx]*)'
+    if not string match -qr "^$basic_pattern(,$basic_pattern)*\$" -- $argv
+        printf (_ "%s: Invalid mask '%s'\n") umask $argv >&2
+        return 1
+    end
+
+    # Split umask into individual digits. We erase the first one because it should always be zero.
+    set -l res (string split '' $umask)
+    set -e res[1]
+
+    for rights in (string split , $argv)
+        set -l match (string match -r "^$basic_pattern\$" $rights)
+        set -l scope $match[2]
+        set -l mode $match[3]
+        set -l perms $match[4]
+        if test -n "$scope" -a -z "$mode"
             printf (_ "%s: Invalid mask '%s'\n") umask $argv >&2
             return 1
         end
-
+        if test -z "$scope"
-        set -l implicit_all
+            set scope a
-
+        end
-        # Insert inverted umask into res variable
+        if test -z "$mode"
-
+            set mode =
-        set -l mode
-        set -l val
-        set -l tmp $umask
-        set -l res
-
-        for i in 1 2 3
-            set tmp (echo $tmp|cut -c 2-)
-            set -l char_count (echo $tmp|cut -c 1)
-            set res[$i] (math 7 - $char_count)
         end
 
-        set -l el (echo $argv|tr , \n)
+        set -l scopes_to_modify
-        for i in $el
+        string match -q '*u*' $scope
-            switch $i
+        and set scopes_to_modify 1
-                case 'u*'
+        string match -q '*g*' $scope
-                    set idx 1
+        and set scopes_to_modify $scopes_to_modify 2
-                    set i (echo $i| cut -c 2-)
+        string match -q '*o*' $scope
+        and set scopes_to_modify $scopes_to_modify 3
+        string match -q '*a*' $scope
+        and set scopes_to_modify 1 2 3
 
-                case 'g*'
+        set -l val 0
-                    set idx 2
+        if string match -q '*r*' $perms
-                    set i (echo $i| cut -c 2-)
+            set val 4
-
+        end
-                case 'o*'
+        if string match -q '*w*' $perms
-                    set idx 3
+            set val (math $val + 2)
-                    set i (echo $i| cut -c 2-)
+        end
-
+        if string match -q '*x*' $perms
-                case 'a*'
+            set val (math $val + 1)
-                    set idx 1 2 3
-                    set i (echo $i| cut -c 2-)
-
-                case '*'
-                    set implicit_all 1
-                    set idx 1 2 3
-            end
-
-            switch $i
-                case '=*'
-                    set mode set
-                    set i (echo $i| cut -c 2-)
-
-                case '+*'
-                    set mode add
-                    set i (echo $i| cut -c 2-)
-
-                case '-*'
-                    set mode remove
-                    set i (echo $i| cut -c 2-)
-
-                case '*'
-                    if not count $implicit_all >/dev/null
-                        printf (_ "%s: Invalid mask '%s'\n") umask $argv >&2
-                        return
-                    end
-                    set mode set
-            end
-
-            if not echo $perm | __fish_sgrep -E '^(r|w|x)*$' >/dev/null
-                printf (_ "%s: Invalid mask '%s'\n") umask $argv >&2
-                return
-            end
-
-            set val 0
-            if echo $i | __fish_sgrep 'r' >/dev/null
-                set val 4
-            end
-            if echo $i | __fish_sgrep 'w' >/dev/null
-                set val (math $val + 2)
-            end
-            if echo $i | __fish_sgrep 'x' >/dev/null
-                set val (math $val + 1)
-            end
-
-            for j in $idx
-                switch $mode
-                    case set
-                        set res[$j] $val
-
-                    case add
-                        set res[$j] (perl -e 'print( ( '$res[$j]'|'$val[$j]' )."\n" )')
-
-                    case remove
-                        set res[$j] (perl -e 'print( ( (7-'$res[$j]')&'$val[$j]' )."\n" )')
-                end
-            end
         end
 
-        for i in 1 2 3
+        for j in $scopes_to_modify
-            set res[$i] (math 7 - $res[$i])
+            switch $mode
+                case '='
+                    set res[$j] (__fish_umask_set $val)
+
+                case '+'
+                    set res[$j] (__fish_umask_add $res[$j] $val)
+
+                case '-'
+                    set res[$j] (__fish_umask_remove $res[$j] $val)
+            end
         end
-        echo 0$res[1]$res[2]$res[3]
     end
+
+    echo 0$res[1]$res[2]$res[3]
+    return 0
 end
 
 function __fish_umask_print_symbolic
+    set -l val
     set -l res ""
     set -l letter a u g o
 
@@ -133,14 +145,12 @@ function __fish_umask_print_symbolic
 
     end
 
-    echo $res | cut -c 2-
+    echo (string split -m 1 '' -- $res)[2]
 end
 
 function umask --description "Set default file permission mask"
-
     set -l as_command 0
     set -l symbolic 0
-
     set -l options
     set -l shortopt pSh
     if not getopt -T >/dev/null
@@ -164,7 +174,6 @@ function umask --description "Set default file permission mask"
     eval set opt $tmp
 
     while count $opt >/dev/null
-
         switch $opt[1]
             case -h --help
                 __fish_print_help umask
@@ -179,15 +188,12 @@ function umask --description "Set default file permission mask"
             case --
                 set -e opt[1]
                 break
-
         end
 
         set -e opt[1]
-
     end
 
     switch (count $opt)
-
         case 0
             if not set -q umask
                 set -g umask 113
@@ -203,8 +209,7 @@ function umask --description "Set default file permission mask"
             end
 
         case 1
-            set -l parsed (__fish_umask_parse $opt)
+            if set -l parsed (__fish_umask_parse $opt)
-            if test (count $parsed) -eq 1
                 set -g umask $parsed
                 return 0
             end
@@ -212,7 +217,6 @@ function umask --description "Set default file permission mask"
 
         case '*'
             printf (_ '%s: Too many arguments\n') umask >&2
-
+            return 1
     end
-
 end

tests/umask.err

@@ -0,0 +1,3 @@
+umask: Invalid mask '1234'
+umask: Invalid mask '228'
+umask: Invalid mask '0282'

tests/umask.in

@@ -0,0 +1,84 @@
+# Test the umask command. In particular the symbolic modes since they've been
+# broken for four years (see issue #738) at the time I added these tests.
+
+# Establish a base line umask.
+umask 027
+umask
+echo umask var = $umask
+umask -S
+
+# Verify that an invalid umask is rejected
+umask 1234
+umask 228
+umask 0282
+
+# Verify that symbolic modifications and output is correct.
+#
+# When I wrote these tests I based all of the results on the behavior of bash
+# when executing identical commands. So if bash has a bug with the umask
+# command it's possible fish will as well. However, I did verify the result of
+# each interaction and did not find any bugs in how bash or fish handled these
+# scenarios.
+#
+echo
+echo running umask a-r
+umask 0777
+umask a-r
+umask
+umask -S
+
+echo
+echo running umask u+x
+umask 0777
+umask u+x
+umask
+umask -S
+
+echo
+echo running umask g+rwx,o+x
+umask 777
+umask g+rwx,o+x
+umask
+umask -S
+
+echo
+echo running umask u-w,o-x
+umask 0
+umask u-w,o-x
+umask
+umask -S
+
+echo
+echo running umask a-r
+umask 0
+umask a-r
+umask
+umask -S
+
+echo
+echo running umask ug-rx
+umask 0
+umask ug-rx
+umask
+umask -S
+
+echo
+echo running umask u+r,g+w,o=rw
+umask 777
+umask u+r,g+w,o=rw
+umask
+umask -S
+
+echo
+echo running umask =r,g+w,o+x,o-r
+umask 777
+umask =r,g+w,o+x,o-r
+umask
+umask -S
+
+echo
+echo running umask rx
+umask 0
+umask rx
+umask
+umask -S

tests/umask.out

@@ -0,0 +1,39 @@
+0027
+umask var = 0027
+u=rwx,g=rx,o=
+
+running umask a-r
+0777
+u=,g=,o=
+
+running umask u+x
+0677
+u=x,g=,o=
+
+running umask g+rwx,o+x
+0706
+u=,g=rwx,o=x
+
+running umask u-w,o-x
+0201
+u=rx,g=rwx,o=rw
+
+running umask a-r
+0444
+u=wx,g=wx,o=wx
+
+running umask ug-rx
+0550
+u=w,g=w,o=rwx
+
+running umask u+r,g+w,o=rw
+0351
+u=r,g=w,o=rw
+
+running umask =r,g+w,o+x,o-r
+0316
+u=r,g=rw,o=x
+
+running umask rx
+0222
+u=rx,g=rx,o=rx