Current implementation of recursive chmod considered harmful

[allisonkarlitskaya]

We currently have a button for "Change Permissions for Enclosed Files" which calls chmod -R with a numeric mode. That's a problem because you usually don't want files and directories to have the same mode. Either all your files end up executable or all of your directories end up broken.

In GNOME this button is labelled "Change Permissions for Enclosed Files..." and opens a second dialog:

If we're going to have recursive mode changing at all, we need something like this. Until that point, I'd argue that we should remove the existing button.

[garrett]

I agree.

[allisonkarlitskaya]

chmod also has a way to say +X which means "add executable permission if any other user has executable permissions". That usually does the "right thing" for both files and directories (and also includes files that are executable files).

If we dumb down the dialog a bit and make it about read/write permissions only, and use +X in the same places we allow read permissions, it might make even more sense than having the full-blown GNOME approach (which breaks existing executable files).

[martinpitt]

Agreed. I also added that to the MVP milestone.

[allisonkarlitskaya]

So here's a very concrete proposal for what I think should happen:

Files and directories should be treated exactly the same, always. To state it explicitly: we should forget about the idea of having "list" (+r) and "access" (+x) treated separately for directories. That's just "read", as far as we're concerned.

In any UI to deal with this, we'll have three categories: user, group, other. Each of those will have a dropdown containing the options "Read and write", "Read-only", "No access":

Access permissions

   User     [  Read and write  | v ]

   Group    [  Read-only       | v ]

   Other    [  No access       | v ]

with each of the dropdowns looking like:

   User     [  Read and write      ]
            |  Read-only           |
            |  No access           |

From this dialog, we construct a string via concatenation, using the following approach:

  "User" = "u"
    "Read and write" = "+rwX"
    "Read-only" = "+rX-w"
    "No access" = "-rwx"
","
  "Group" = "g"
    ... as for the options for user ...
","
  "Other" = "o"
    ... as for the options for user ...

So given the example above, the resulting string would be u+rwX,g+rX-w,o-rwx. This is suitable for passing to chmod or chmod -R for any file or directory.

The only thing that's missing here is support for setting the executable bit on a script. I think that this option does not make sense to be applied recursively, but might make sense on a single file or multiple-selection containing only files (not directories). We might add a separate checkbox for it in this case, in which case we'd see something like (for the single file case):

Access permissions

   User     [  Read and write  | v ]

   Group    [  Read-only       | v ]

   Other    [  No access       | v ]

[x] This file is executable.

This box would be checked if any executable bit is set on the file when the dialog is open. In the case that this explicit UI element is visible we could modify our mode string construction behaviour:

• [x] This file is executable:
  • Read and write: +rwx
  • Read-only: +rx-w
  • No access: -rwx
• [ ] This file is executable:
  • Read and write: +rw-x
  • Read-only: +r-wx
  • No access: -rwx

This leaves only the case about what we do when we select multiple files (not directories) and modify their attributes all at once and the selected set has inconsistent executable bits set on it from the beginning.

We could either refuse to handle this case (not show the check) or handle it by showing a checkbox in an inconsistent state (does Patternfly have that?). If the user leaves the check in the inconsistent state then we proceed as per the case where the option wasn't shown.

[allisonkarlitskaya]

a checkbox in an inconsistent state (does Patternfly have that?)

https://www.patternfly.org/components/forms/checkbox/

isChecked boolean | null false Flag to show if the checkbox is checked. If null, the checkbox will be indeterminate (partially checked).

[jelly]

a checkbox in an inconsistent state (does Patternfly have that?)

https://www.patternfly.org/components/forms/checkbox/

isChecked boolean | null false Flag to show if the checkbox is checked. If null, the checkbox will be indeterminate (partially checked).

No, use a Switch https://www.patternfly.org/components/switch/#checked-with-label :)

[martinpitt]

@jelly says that this functionality doesn't exist, so it doesn't need to block the first release.