It's useful to delete folders other than the current one. If a folder is
specified, delete that one; otherwise, delete the current one.
Changelog-added: Allow specifying the folder to delete with `:rmdir`.
Signed-off-by: Jason Cox <me@jasoncarloscox.com>
---
v1 -> v2: Leave the logic to find a new directory to open as-is instead
of refactoring it into a separate function. This tweak
significantly reduces the diff size as requested by Moritz.
Also move the remove function out of the Execute function to
reduce indentation.
commands/account/rmdir.go | 66 +++++++++++++++++++++++++++------------doc/aerc.1.scd | 4 +--
2 files changed, 48 insertions(+), 22 deletions(-)
diff --git a/commands/account/rmdir.go b/commands/account/rmdir.go
index 22e78a69..1f8854fe 100644
--- a/commands/account/rmdir.go+++ b/commands/account/rmdir.go
@@ -2,16 +2,19 @@ package account
import (
"errors"
+ "fmt" "time"
"git.sr.ht/~rjarry/aerc/app"
"git.sr.ht/~rjarry/aerc/commands"
"git.sr.ht/~rjarry/aerc/models"
"git.sr.ht/~rjarry/aerc/worker/types"
+ "git.sr.ht/~rjarry/go-opt")
type RemoveDir struct {
- Force bool `opt:"-f"`+ Force bool `opt:"-f"`+ Folder string `opt:"folder" complete:"CompleteFolder" required:"false"`}
func init() {
@@ -26,17 +29,31 @@ func (RemoveDir) Aliases() []string {
return []string{"rmdir"}
}
+func (RemoveDir) CompleteFolder(arg string) []string {+ acct := app.SelectedAccount()+ if acct == nil {+ return nil+ }+ return commands.FilterList(acct.Directories().List(), arg, opt.QuoteArg)+}+func (r RemoveDir) Execute(args []string) error {
acct := app.SelectedAccount()
if acct == nil {
return errors.New("No account selected")
}
- var role models.Role- if d := acct.Directories().SelectedDirectory(); d != nil {- role = d.Role+ current := acct.Directories().SelectedDirectory()+ toRemove := current+ if r.Folder != "" {+ toRemove = acct.Directories().Directory(r.Folder)+ if toRemove == nil {+ return fmt.Errorf("No such directory: %s", r.Folder)+ } }
+ role := toRemove.Role+ // Check for any messages in the directory.
if role != models.QueryRole && !acct.Messages().Empty() && !r.Force {
return errors.New("Refusing to remove non-empty directory; use -f")
@@ -46,7 +63,12 @@ func (r RemoveDir) Execute(args []string) error {
return errors.New("Cannot remove a virtual node")
}
- curDir := acct.SelectedDirectory()+ if toRemove != current {+ r.remove(acct, toRemove, func() {})+ return nil+ }++ curDir := current.Name var newDir string
dirFound := false
@@ -102,22 +124,26 @@ func (r RemoveDir) Execute(args []string) error {
default:
return
}
- acct.Worker().PostAction(&types.RemoveDirectory{- Directory: curDir,- Quiet: r.Force,- }, func(msg types.WorkerMessage) {- switch msg := msg.(type) {- case *types.Done:- app.PushStatus("Directory removed.", 10*time.Second)- case *types.Error:- app.PushError(msg.Error.Error())- reopenCurrentDir()- case *types.Unsupported:- app.PushError(":rmdir is not supported by the backend.")- reopenCurrentDir()- }- })+ r.remove(acct, toRemove, reopenCurrentDir) })
return nil
}
++func (r RemoveDir) remove(acct *app.AccountView, dir *models.Directory, onErr func()) {+ acct.Worker().PostAction(&types.RemoveDirectory{+ Directory: dir.Name,+ Quiet: r.Force,+ }, func(msg types.WorkerMessage) {+ switch msg := msg.(type) {+ case *types.Done:+ app.PushStatus("Directory removed.", 10*time.Second)+ case *types.Error:+ app.PushError(msg.Error.Error())+ onErr()+ case *types.Unsupported:+ app.PushError(":rmdir is not supported by the backend.")+ onErr()+ }+ })+}
diff --git a/doc/aerc.1.scd b/doc/aerc.1.scd
index 62129bff..8d489fb9 100644
--- a/doc/aerc.1.scd+++ b/doc/aerc.1.scd
@@ -577,8 +577,8 @@ message list, the message in the message viewer, etc).
*:mkdir* _<name>_
Creates a new folder for this account and changes to that folder.
-*:rmdir* [*-f*]- Removes the current folder.+*:rmdir* [*-f*] [_<folder>_]+ Removes the folder _<folder>_, or the current folder if not specified. By default, it will fail if the directory is non-empty (see *-f*).
--
2.45.1
Hi Jason
On Fri Jun 14, 2024 at 12:22 PM CEST, Jason Cox wrote:
> + role := toRemove.Role> +> // Check for any messages in the directory.> if role != models.QueryRole && !acct.Messages().Empty() && !r.Force {
We check with `acct.Messages().Empty()` if the folder that should be
removed is empty. However, using `acct.Messages()` will always look at
the message list of the *current* folder. If the current folder is
empty, you could remove any other folder that contains messages without
the force flag.
You can reproduce with
1) create a new folder:
:mkdir test
which creates an empty folder and selects it
2) then you can remove any other non-empty directory without the force flag:
:rmdir <a-non-empty-folder>
I think we should explicitly check the folder that will be removed
whether it is empty or not?
--
Koni
On Sat Jun 15, 2024 at 5:27 AM EDT, Koni Marti wrote:
> On Fri Jun 14, 2024 at 12:22 PM CEST, Jason Cox wrote:> > + role := toRemove.Role> > +> > // Check for any messages in the directory.> > if role != models.QueryRole && !acct.Messages().Empty() && !r.Force {>> We check with `acct.Messages().Empty()` if the folder that should be> removed is empty. However, using `acct.Messages()` will always look at> the message list of the *current* folder. If the current folder is> empty, you could remove any other folder that contains messages without> the force flag.>> You can reproduce with>> 1) create a new folder: >> :mkdir test>> which creates an empty folder and selects it>> 2) then you can remove any other non-empty directory without the force flag: >> :rmdir <a-non-empty-folder>>> I think we should explicitly check the folder that will be removed> whether it is empty or not?
Yes, good catch! From my testing it seems that checking if
toRemove.Exists > 0 should do it, right?
On Sun Jun 16, 2024 at 2:57 AM CEST, Jason Cox wrote:
> On Sat Jun 15, 2024 at 5:27 AM EDT, Koni Marti wrote:> > On Fri Jun 14, 2024 at 12:22 PM CEST, Jason Cox wrote:> > > + role := toRemove.Role> > > +> > > // Check for any messages in the directory.> > > if role != models.QueryRole && !acct.Messages().Empty() && !r.Force {> >> > We check with `acct.Messages().Empty()` if the folder that should be> > removed is empty. However, using `acct.Messages()` will always look at> > the message list of the *current* folder. If the current folder is> > empty, you could remove any other folder that contains messages without> > the force flag.> >> > You can reproduce with> >> > 1) create a new folder: > >> > :mkdir test> >> > which creates an empty folder and selects it> >> > 2) then you can remove any other non-empty directory without the force flag: > >> > :rmdir <a-non-empty-folder>> >> > I think we should explicitly check the folder that will be removed> > whether it is empty or not? >> Yes, good catch! From my testing it seems that checking if> toRemove.Exists > 0 should do it, right?
Yes, I think that's a good approach.
--
Koni