9front - general discussion about 9front
 help / color / mirror / Atom feed
* [9front] Mail rewrite; open path with stored mails
@ 2021-02-12 20:54 theinicke
  2021-02-12 21:10 ` ori
  0 siblings, 1 reply; 12+ messages in thread
From: theinicke @ 2021-02-12 20:54 UTC (permalink / raw)
  To: 9front

I have noticed that Mail/Nail does no longer open additional mailboxes if passed a path [containing mails as undecoded MIME messages]. Now I am wondering whether this is by design or it is desirable to reintroduce this.

That said I can imagine some possibilities for reintroduction:

1) change argv parsing to interpret argument like old Mail did (e.g. mbox | path)
1a) also reintroduce second optional argument to open mailbox with given mboxname - imho unnecessary, but backwards compatibility?
2) introduce this via an additional parameter
3) just do it using a script

I could provide a patch implementing it, but I am not sure whether this was left out by design (e.g. because if one needed it one could easily do this with a script) and which solution to favor, thoughts?

--
Tobias Heinicke


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [9front] Mail rewrite; open path with stored mails
  2021-02-12 20:54 [9front] Mail rewrite; open path with stored mails theinicke
@ 2021-02-12 21:10 ` ori
  2021-02-13 20:47   ` theinicke
  0 siblings, 1 reply; 12+ messages in thread
From: ori @ 2021-02-12 21:10 UTC (permalink / raw)
  To: 9front

Quoth theinicke@bss-wf.de:
> I have noticed that Mail/Nail does no longer open additional mailboxes if passed a path [containing mails as undecoded MIME messages]. Now I am wondering whether this is by design or it is desirable to reintroduce this.
> 
> That said I can imagine some possibilities for reintroduction:
> 
> 1) change argv parsing to interpret argument like old Mail did (e.g. mbox | path)
> 1a) also reintroduce second optional argument to open mailbox with given mboxname - imho unnecessary, but backwards compatibility?
> 2) introduce this via an additional parameter
> 3) just do it using a script

The goal wasn't for perfect compatibility,
but I have nothing against the feature. I
just didn't use it, and nobody complained
that it was gone until now.

I'd be happy to take a patch for this.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [9front] Mail rewrite; open path with stored mails
  2021-02-12 21:10 ` ori
@ 2021-02-13 20:47   ` theinicke
  2021-02-13 21:25     ` theinicke
  0 siblings, 1 reply; 12+ messages in thread
From: theinicke @ 2021-02-13 20:47 UTC (permalink / raw)
  To: 9front

Quoth ori@eigenstate.org:
> Quoth theinicke@bss-wf.de:
> > I have noticed that Mail/Nail does no longer open additional mailboxes if passed a path [containing mails as undecoded MIME messages]. Now I am wondering whether this is by design or it is desirable to reintroduce this.
> > 
> > That said I can imagine some possibilities for reintroduction:
> > 
> > 1) change argv parsing to interpret argument like old Mail did (e.g. mbox | path)
> > 1a) also reintroduce second optional argument to open mailbox with given mboxname - imho unnecessary, but backwards compatibility?
> > 2) introduce this via an additional parameter
> > 3) just do it using a script
> 
> The goal wasn't for perfect compatibility,
> but I have nothing against the feature. I
> just didn't use it, and nobody complained
> that it was gone until now.
> 
> I'd be happy to take a patch for this.

Thank you for your reply and sorry for the noise of accidentally sending multiple mails, I have inlined a patch which introduces it via a new parameter. Compared to old Mail this allows us to work with relative paths (on fs).

diff -r 02e3059af5bc sys/man/1/acmemail
--- a/sys/man/1/acmemail	Thu Feb 11 09:37:36 2021 +0100
+++ b/sys/man/1/acmemail	Sat Feb 13 21:40:11 2021 +0100
@@ -20,6 +20,10 @@
 .B -o
 .I outbox
 ]
+[
+.B -d
+.I path
+]
 
 .SH DESCRIPTION
 .PP
@@ -64,6 +68,10 @@
 Save a copy of outgoing messages to the mailbox
 .IR outbox ,
 instead of discarding them after they're enqueued.
+.TP
+.BI -d " path
+Opens new mailbox for the given path,
+closes it on exit.
 
 .PP
 Mail presents and acme interface for a upas/fs mailbox.
diff -r 02e3059af5bc sys/src/cmd/upas/Mail/mbox.c
--- a/sys/src/cmd/upas/Mail/mbox.c	Thu Feb 11 09:37:36 2021 +0100
+++ b/sys/src/cmd/upas/Mail/mbox.c	Sat Feb 13 21:39:50 2021 +0100
@@ -32,6 +32,8 @@
 
 Reprog	*mesgpat;
 
+int	openedmbox = 0;
+
 int	threadsort = 1;
 int	sender;
 
@@ -51,8 +53,9 @@
 	Plumbmsg *m;
 
 	while(1){
-		if((m = plumbrecv(fd)) == nil)
+		if((m = plumbrecv(fd)) == nil) {
 			threadexitsall("plumber gone");
+		}
 		sendp(ch, m);
 	}
 }
@@ -672,17 +675,26 @@
 	}
 }
 
+static int
+openctlfile(void)
+{
+	int fd;
+	char *path;
+
+	path = estrjoin(maildir, "/ctl", nil);
+	fd = open(path, OWRITE);
+	free(path);
+	return fd;
+}
+
 static void
 mbflush(char **, int)
 {
 	int i, j, ln, fd;
-	char *path;
 	Mesg *m, *p;
 
 	i = 0;
-	path = estrjoin(maildir, "/ctl", nil);
-	fd = open(path, OWRITE);
-	free(path);
+	fd = openctlfile();
 	if(fd == -1)
 		sysfatal("open mbox: %r");
 	while(i < mbox.nmesg){
@@ -753,6 +765,22 @@
 }
 
 static void
+removeopened(void)
+{
+	int fd;
+	char buf[256];
+
+	if(!openedmbox)
+		return;
+	fd = openctlfile();
+	if(fd == -1)
+		return;
+
+	snprint(buf, sizeof buf, "close %s", mailbox);
+	write(fd, buf, strlen(buf));
+}
+
+static void
 quitall(char **, int)
 {
 	Mesg *m;
@@ -768,6 +796,7 @@
 	for(c = mbox.opencomp; c != nil; c = c->qnext)
 		fprint(c->ctl, "del\n");
 	fprint(mbox.ctl, "del\n");
+	removeopened();
 	threadexitsall(nil);
 }
 
@@ -975,15 +1004,81 @@
 static void
 usage(void)
 {
-	fprint(2, "usage: %s [-T] [-m mailfs] [-s] [-f format] [mbox]\n", argv0);
+	fprint(2, "usage: %s [-T] [-m mailfs] [-s] [-f format] [-d path] [mbox]\n", argv0);
 	exits("usage");
 }
 
+static void
+openmbox(char *path, char *mboxname)
+{
+	int fd, i;
+	char buf[512], err[ERRMAX];
+	char *name, *fsname, *s1, *s2, *abspath;
+
+	/* if path is already absolute, leave it as it is - to allow /imap...
+	otherwise get absolute path from relative one */
+	if(*path == '/'){
+		i = strlen(path);
+		if(path[i-1] == '/')
+			path[i-1] = '\0';
+		abspath = estrdup(path);
+	}
+	else {
+		fd = open(path, OREAD);
+		if(fd < 0)
+			sysfatal("can't open %s: %r", path);
+	
+		if(fd2path(fd, buf, sizeof buf) != 0)
+			sysfatal("fd2path %s: %r", path);
+		close(fd);
+		abspath = estrdup(buf);
+	}
+
+	if(mboxname != nil)
+		name = mboxname;
+	else{
+		s1 = strrchr(abspath, '/');
+		if(s1 == nil)
+			name = abspath;
+		else{
+			*s1++ = '\0';
+			name = s1;
+			*--s1 = '/';
+		}
+	}
+
+	fd = openctlfile();
+	if(fd == -1)
+		sysfatal("open mbox: %r");
+	fsname = estrdup(name);
+	s2 = emalloc(5+strlen(abspath)+1+strlen(fsname)+2+1);
+	for(i=0; i<10; i++){
+		sprint(s2, "open %s %s", abspath, fsname);
+		if(write(fd, s2, strlen(s2)) >= 0)
+			break;
+		err[0] = '\0';
+		errstr(err, sizeof err);
+		if(strstr(err, "mbox name in use") == nil)
+			sysfatal("can't create directory %s for mail: %s", name, err);
+		free(fsname);
+		fsname = emalloc(strlen(name)+3);
+		sprint(fsname, "%s-%d", name, i);
+	}
+	if(i == 10)
+		sysfatal("can't open %s: %r", abspath);
+	mailbox = fsname;
+	openedmbox = 1;
+	free(s2);
+	free(abspath);
+	close(fd);
+}
+
 void
 threadmain(int argc, char **argv)
 {
 	Fmt fmt;
 	char *cmd;
+	char *path = nil, *mboxname = nil;
 	int i;
 
 	mbox.view = Vgroup;
@@ -1017,6 +1112,9 @@
 	case 'o':
 		savebox = EARGF(usage());
 		break;
+	case 'd':
+		path = EARGF(usage());
+		break;
 	default:
 		usage();
 		break;
@@ -1024,8 +1122,13 @@
 
 	if(argc > 1)
 		usage();
-	if(argc == 1)
+	if(argc == 1) {
 		mailbox = argv[0];
+		mboxname = mailbox;
+	}
+
+	if(path != nil)
+		openmbox(path, mboxname);
 
 	mesgpat = regcomp("([0-9]+)(/.*)?");
 	cwait = threadwaitchan();


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [9front] Mail rewrite; open path with stored mails
  2021-02-13 20:47   ` theinicke
@ 2021-02-13 21:25     ` theinicke
  2021-02-13 23:59       ` Alex Musolino
  0 siblings, 1 reply; 12+ messages in thread
From: theinicke @ 2021-02-13 21:25 UTC (permalink / raw)
  To: 9front

Sorry - please consider following patch instead, as it also attempts to close opened mailbox, if Mail exits due to plumber being gone.

--
Tobias Heinicke

diff -r 02e3059af5bc sys/src/cmd/upas/Mail/mbox.c
--- a/sys/src/cmd/upas/Mail/mbox.c	Thu Feb 11 09:37:36 2021 +0100
+++ b/sys/src/cmd/upas/Mail/mbox.c	Sat Feb 13 22:21:29 2021 +0100
@@ -32,6 +32,8 @@
 
 Reprog	*mesgpat;
 
+int	openedmbox = 0;
+
 int	threadsort = 1;
 int	sender;
 
@@ -45,14 +47,44 @@
 
 static void	showmesg(Biobuf*, Mesg*, int, int);
 
+static int
+openctlfile(void)
+{
+	int fd;
+	char *path;
+
+	path = estrjoin(maildir, "/ctl", nil);
+	fd = open(path, OWRITE);
+	free(path);
+	return fd;
+}
+
+static void
+removeopened(void)
+{
+	int fd;
+	char buf[256];
+
+	if(!openedmbox)
+		return;
+	fd = openctlfile();
+	if(fd == -1)
+		return;
+
+	snprint(buf, sizeof buf, "close %s", mailbox);
+	write(fd, buf, strlen(buf));
+}
+
 static void
 plumbloop(Channel *ch, int fd)
 {
 	Plumbmsg *m;
 
 	while(1){
-		if((m = plumbrecv(fd)) == nil)
+		if((m = plumbrecv(fd)) == nil) {
+			removeopened();
 			threadexitsall("plumber gone");
+		}
 		sendp(ch, m);
 	}
 }
@@ -676,13 +708,10 @@
 mbflush(char **, int)
 {
 	int i, j, ln, fd;
-	char *path;
 	Mesg *m, *p;
 
 	i = 0;
-	path = estrjoin(maildir, "/ctl", nil);
-	fd = open(path, OWRITE);
-	free(path);
+	fd = openctlfile();
 	if(fd == -1)
 		sysfatal("open mbox: %r");
 	while(i < mbox.nmesg){
@@ -768,6 +797,7 @@
 	for(c = mbox.opencomp; c != nil; c = c->qnext)
 		fprint(c->ctl, "del\n");
 	fprint(mbox.ctl, "del\n");
+	removeopened();
 	threadexitsall(nil);
 }
 
@@ -975,15 +1005,80 @@
 static void
 usage(void)
 {
-	fprint(2, "usage: %s [-T] [-m mailfs] [-s] [-f format] [mbox]\n", argv0);
+	fprint(2, "usage: %s [-T] [-m mailfs] [-s] [-f format] [-d path] [mbox]\n", argv0);
 	exits("usage");
 }
 
+static void
+openmbox(char *path, char *mboxname)
+{
+	int fd, i;
+	char buf[512], err[ERRMAX];
+	char *name, *fsname, *s1, *s2, *abspath;
+
+	/* if path is already absolute, leave it as it is - to allow /imap...
+	otherwise get absolute path from relative one */
+	if(*path == '/'){
+		i = strlen(path);
+		if(path[i-1] == '/')
+			path[i-1] = '\0';
+		abspath = estrdup(path);
+	}else{
+		fd = open(path, OREAD);
+		if(fd < 0)
+			sysfatal("can't open %s: %r", path);
+	
+		if(fd2path(fd, buf, sizeof buf) != 0)
+			sysfatal("fd2path %s: %r", path);
+		close(fd);
+		abspath = estrdup(buf);
+	}
+
+	if(mboxname != nil)
+		name = mboxname;
+	else{
+		s1 = strrchr(abspath, '/');
+		if(s1 == nil)
+			name = abspath;
+		else{
+			*s1++ = '\0';
+			name = s1;
+			*--s1 = '/';
+		}
+	}
+
+	fd = openctlfile();
+	if(fd == -1)
+		sysfatal("open mbox: %r");
+	fsname = estrdup(name);
+	s2 = emalloc(5+strlen(abspath)+1+strlen(fsname)+2+1);
+	for(i=0; i<10; i++){
+		sprint(s2, "open %s %s", abspath, fsname);
+		if(write(fd, s2, strlen(s2)) >= 0)
+			break;
+		err[0] = '\0';
+		errstr(err, sizeof err);
+		if(strstr(err, "mbox name in use") == nil)
+			sysfatal("can't create directory %s for mail: %s", name, err);
+		free(fsname);
+		fsname = emalloc(strlen(name)+3);
+		sprint(fsname, "%s-%d", name, i);
+	}
+	if(i == 10)
+		sysfatal("can't open %s: %r", abspath);
+	mailbox = fsname;
+	openedmbox = 1;
+	free(s2);
+	free(abspath);
+	close(fd);
+}
+
 void
 threadmain(int argc, char **argv)
 {
 	Fmt fmt;
 	char *cmd;
+	char *path = nil, *mboxname = nil;
 	int i;
 
 	mbox.view = Vgroup;
@@ -1017,6 +1112,9 @@
 	case 'o':
 		savebox = EARGF(usage());
 		break;
+	case 'd':
+		path = EARGF(usage());
+		break;
 	default:
 		usage();
 		break;
@@ -1024,8 +1122,13 @@
 
 	if(argc > 1)
 		usage();
-	if(argc == 1)
+	if(argc == 1) {
 		mailbox = argv[0];
+		mboxname = mailbox;
+	}
+
+	if(path != nil)
+		openmbox(path, mboxname);
 
 	mesgpat = regcomp("([0-9]+)(/.*)?");
 	cwait = threadwaitchan();
diff -r 02e3059af5bc sys/man/1/acmemail
--- a/sys/man/1/acmemail	Thu Feb 11 09:37:36 2021 +0100
+++ b/sys/man/1/acmemail	Sat Feb 13 22:23:35 2021 +0100
@@ -20,6 +20,10 @@
 .B -o
 .I outbox
 ]
+[
+.B -d
+.I path
+]
 
 .SH DESCRIPTION
 .PP
@@ -64,6 +68,10 @@
 Save a copy of outgoing messages to the mailbox
 .IR outbox ,
 instead of discarding them after they're enqueued.
+.TP
+.BI -d " path
+Opens new mailbox for the given path,
+closes it on exit.
 
 .PP
 Mail presents and acme interface for a upas/fs mailbox.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [9front] Mail rewrite; open path with stored mails
  2021-02-13 21:25     ` theinicke
@ 2021-02-13 23:59       ` Alex Musolino
  2021-02-14 10:42         ` theinicke
  0 siblings, 1 reply; 12+ messages in thread
From: Alex Musolino @ 2021-02-13 23:59 UTC (permalink / raw)
  To: 9front

I have a couple of general comments.

Firstly, the -d option is typically used as to enable debugging output
of some kind.  Can you use a different character?  Perhaps -p for
path?  Also, it seems that the options we support and the usage
string(s) do not agree.

Secondly, you can register the removembox function to run on exit with
atexit(2).  That'll save you having to call it yourself from any and
every place that might result in an exit.  At the moment, removembox
won't be called if we hit one of the sysfatals.

--
Cheers,
Alex Musolino


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [9front] Mail rewrite; open path with stored mails
  2021-02-13 23:59       ` Alex Musolino
@ 2021-02-14 10:42         ` theinicke
  2021-02-14 20:57           ` ori
  2021-02-14 22:55           ` Alex Musolino
  0 siblings, 2 replies; 12+ messages in thread
From: theinicke @ 2021-02-14 10:42 UTC (permalink / raw)
  To: 9front

Quoth Alex Musolino <alex@musolino.id.au>:
> I have a couple of general comments.
> 
> Firstly, the -d option is typically used as to enable debugging output
> of some kind.  Can you use a different character?  Perhaps -p for
> path?  Also, it seems that the options we support and the usage
> string(s) do not agree.
> 
> Secondly, you can register the removembox function to run on exit with
> atexit(2).  That'll save you having to call it yourself from any and
> every place that might result in an exit.  At the moment, removembox
> won't be called if we hit one of the sysfatals.
> 
> --
> Cheers,
> Alex Musolino
> 

Thank you for your comments, based on your suggestions I have provided the following patch
(assuming that you meant the usage string discrepancy that was there before my patch -
while at it I have also changed NAIL(1) to ACMEMAIL(1); if this was not what you meant please elaborate).

--
Cheers,
Tobias Heinicke


diff -r 02e3059af5bc sys/man/1/acmemail
--- a/sys/man/1/acmemail	Thu Feb 11 09:37:36 2021 +0100
+++ b/sys/man/1/acmemail	Sun Feb 14 11:36:18 2021 +0100
@@ -1,4 +1,4 @@
-.TH NAIL 1
+.TH ACMEMAIL 1
 .SH NAME
 Mail \- view mail in acme
 
@@ -6,7 +6,7 @@
 
 Mail
 [
-.B -OsT
+.B -TOs
 ]
 [
 .B -m
@@ -20,6 +20,13 @@
 .B -o
 .I outbox
 ]
+[
+.B -p
+.I path
+]
+[
+.I mbox
+]
 
 .SH DESCRIPTION
 .PP
@@ -64,6 +71,10 @@
 Save a copy of outgoing messages to the mailbox
 .IR outbox ,
 instead of discarding them after they're enqueued.
+.TP
+.BI -p " path
+Opens new mailbox for the given path,
+closes it on exit.
 
 .PP
 Mail presents and acme interface for a upas/fs mailbox.
diff -r 02e3059af5bc sys/src/cmd/upas/Mail/mbox.c
--- a/sys/src/cmd/upas/Mail/mbox.c	Thu Feb 11 09:37:36 2021 +0100
+++ b/sys/src/cmd/upas/Mail/mbox.c	Sun Feb 14 11:36:18 2021 +0100
@@ -32,6 +32,8 @@
 
 Reprog	*mesgpat;
 
+int	openedmbox = 0;
+
 int	threadsort = 1;
 int	sender;
 
@@ -45,6 +47,34 @@
 
 static void	showmesg(Biobuf*, Mesg*, int, int);
 
+int
+openctlfile(void)
+{
+	int fd;
+	char *path;
+
+	path = estrjoin(maildir, "/ctl", nil);
+	fd = open(path, OWRITE);
+	free(path);
+	return fd;
+}
+
+void
+removeopened(void)
+{
+	int fd;
+	char buf[256];
+
+	if(!openedmbox)
+		return;
+	fd = openctlfile();
+	if(fd == -1)
+		return;
+
+	snprint(buf, sizeof buf, "close %s", mailbox);
+	write(fd, buf, strlen(buf));
+}
+
 static void
 plumbloop(Channel *ch, int fd)
 {
@@ -676,13 +706,10 @@
 mbflush(char **, int)
 {
 	int i, j, ln, fd;
-	char *path;
 	Mesg *m, *p;
 
 	i = 0;
-	path = estrjoin(maildir, "/ctl", nil);
-	fd = open(path, OWRITE);
-	free(path);
+	fd = openctlfile();
 	if(fd == -1)
 		sysfatal("open mbox: %r");
 	while(i < mbox.nmesg){
@@ -975,15 +1002,84 @@
 static void
 usage(void)
 {
-	fprint(2, "usage: %s [-T] [-m mailfs] [-s] [-f format] [mbox]\n", argv0);
+	fprint(2, "usage: %s [-TOs] [-m maildir] [-f format] [-o outbox] [-p path] [mbox]\n", argv0);
 	exits("usage");
 }
 
+static void
+openmbox(char *path, char *mboxname)
+{
+	int fd, i;
+	char buf[512], err[ERRMAX];
+	char *name, *fsname, *s1, *s2, *abspath;
+
+	/* if path is already absolute, leave it as it is - to allow /imap...
+	otherwise get absolute path from relative one */
+	if(*path == '/'){
+		i = strlen(path);
+		if(path[i-1] == '/')
+			path[i-1] = '\0';
+		abspath = estrdup(path);
+	}else{
+		fd = open(path, OREAD);
+		if(fd < 0)
+			sysfatal("can't open %s: %r", path);
+	
+		if(fd2path(fd, buf, sizeof buf) != 0)
+			sysfatal("fd2path %s: %r", path);
+		close(fd);
+		abspath = estrdup(buf);
+	}
+
+	if(mboxname != nil)
+		name = mboxname;
+	else{
+		s1 = strrchr(abspath, '/');
+		if(s1 == nil)
+			name = abspath;
+		else{
+			*s1++ = '\0';
+			name = s1;
+			*--s1 = '/';
+		}
+	}
+
+	fd = openctlfile();
+	if(fd == -1)
+		sysfatal("open mbox: %r");
+	fsname = estrdup(name);
+	s2 = emalloc(5+strlen(abspath)+1+strlen(fsname)+2+1);
+	for(i=0; i<10; i++){
+		sprint(s2, "open %s %s", abspath, fsname);
+		if(write(fd, s2, strlen(s2)) >= 0)
+			break;
+		err[0] = '\0';
+		errstr(err, sizeof err);
+		if(strstr(err, "mbox name in use") == nil)
+			sysfatal("can't create directory %s for mail: %s", name, err);
+		free(fsname);
+		fsname = emalloc(strlen(name)+3);
+		sprint(fsname, "%s-%d", name, i);
+	}
+	if(i == 10)
+		sysfatal("can't open %s: %r", abspath);
+	if(atexit(removeopened) == 0) {
+		removeopened();
+		sysfatal("atexit: %r");
+	}
+	mailbox = fsname;
+	openedmbox = 1;
+	free(s2);
+	free(abspath);
+	close(fd);
+}
+
 void
 threadmain(int argc, char **argv)
 {
 	Fmt fmt;
 	char *cmd;
+	char *path = nil, *mboxname = nil;
 	int i;
 
 	mbox.view = Vgroup;
@@ -1017,6 +1113,9 @@
 	case 'o':
 		savebox = EARGF(usage());
 		break;
+	case 'p':
+		path = EARGF(usage());
+		break;
 	default:
 		usage();
 		break;
@@ -1024,8 +1123,13 @@
 
 	if(argc > 1)
 		usage();
-	if(argc == 1)
+	if(argc == 1) {
 		mailbox = argv[0];
+		mboxname = mailbox;
+	}
+
+	if(path != nil)
+		openmbox(path, mboxname);
 
 	mesgpat = regcomp("([0-9]+)(/.*)?");
 	cwait = threadwaitchan();


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [9front] Mail rewrite; open path with stored mails
  2021-02-14 10:42         ` theinicke
@ 2021-02-14 20:57           ` ori
  2021-02-14 22:05             ` theinicke
  2021-02-14 22:55           ` Alex Musolino
  1 sibling, 1 reply; 12+ messages in thread
From: ori @ 2021-02-14 20:57 UTC (permalink / raw)
  To: 9front

> Thank you for your comments, based on your suggestions I have provided the following patch
> (assuming that you meant the usage string discrepancy that was there before my patch -
> while at it I have also changed NAIL(1) to ACMEMAIL(1); if this was not what you meant please elaborate).

Some comments -- sorry if this is long:

>  .SH DESCRIPTION
>  .PP
> @@ -64,6 +71,10 @@
>  Save a copy of outgoing messages to the mailbox
>  .IR outbox ,
>  instead of discarding them after they're enqueued.
> +.TP
> +.BI -p " path
> +Opens new mailbox for the given path,
> +closes it on exit.

Why not just make this flag modify the interpretation
of argv[0]?

	Mail -o /path/to/mbox
 

> +void
> +removeopened(void)

closembox()?

> +	fd = openctlfile();
> +	if(fd == -1)
> +		return;

Nitpicking here, but most of the code
uses:

	if((fd = open...) == -1)

> +	snprint(buf, sizeof buf, "close %s", mailbox);
> +	write(fd, buf, strlen(buf));

cleaner:

	fprint(fd, "close %s");

> -	fprint(2, "usage: %s [-T] [-m mailfs] [-s] [-f format] [mbox]\n", argv0);
> +	fprint(2, "usage: %s [-TOs] [-m maildir] [-f format] [-o outbox] [-p path] [mbox]\n", argv0);
>  	exits("usage");
>  }
>  
> +static void
> +openmbox(char *path, char *mboxname)
> +{

This function looks pretty ugly; I'd rather just
have it take a path to the mbox and not mangle it.

> +	int fd, i;
> +	char buf[512], err[ERRMAX];
> +	char *name, *fsname, *s1, *s2, *abspath;
> +
> +	/* if path is already absolute, leave it as it is - to allow /imap...
> +	otherwise get absolute path from relative one */

There's no real harm in skipping this special case.
It's one open in code that does an open for every
mail in the mailbox.

> +	if(*path == '/'){
> +		i = strlen(path);
> +		if(path[i-1] == '/')
> +			path[i-1] = '\0';
> +		abspath = estrdup(path);
> +	}else{
> +		fd = open(path, OREAD);
> +		if(fd < 0)
> +			sysfatal("can't open %s: %r", path);
> +	
> +		if(fd2path(fd, buf, sizeof buf) != 0)
> +			sysfatal("fd2path %s: %r", path);
> +		close(fd);
> +		abspath = estrdup(buf);
> +	}
> +
> +	if(mboxname != nil)
> +		name = mboxname;
> +	else{
> +		s1 = strrchr(abspath, '/');
> +		if(s1 == nil)
> +			name = abspath;
> +		else{
> +			*s1++ = '\0';
> +			name = s1;
> +			*--s1 = '/';

Why the termination and unterminaton?

> +		}
> +	}
> +
> +	fd = openctlfile();
> +	if(fd == -1)
> +		sysfatal("open mbox: %r");
> +	fsname = estrdup(name);
> +	s2 = emalloc(5+strlen(abspath)+1+strlen(fsname)+2+1);
> +	for(i=0; i<10; i++){

Why is this being repeated 10 times? I don't see
anything that is going to be improved by doing
it over and over in rapid succession.

If the mbox is in use, it's almost certain to stay
in use for longer than this loop runs.

> +		sprint(s2, "open %s %s", abspath, fsname);

sprint should probably not be used in new code;
while it's possible to use safely, it's easier
to verify snprint and smprint.

In this case, smprint is probably the right
choice.

> +		if(write(fd, s2, strlen(s2)) >= 0)
> +			break;

Or better: fprint.

>  void
>  threadmain(int argc, char **argv)
>  {
>  	Fmt fmt;
>  	char *cmd;
> +	char *path = nil, *mboxname = nil;
>  	int i;
>  
>  	mbox.view = Vgroup;
> @@ -1017,6 +1113,9 @@
>  	case 'o':
>  		savebox = EARGF(usage());
>  		break;
> +	case 'p':
> +		path = EARGF(usage());
> +		break;
>  	default:
>  		usage();
>  		break;
> @@ -1024,8 +1123,13 @@
>  
>  	if(argc > 1)
>  		usage();
> -	if(argc == 1)
> +	if(argc == 1) {
>  		mailbox = argv[0];
> +		mboxname = mailbox;
> +	}
> +
> +	if(path != nil)
> +		openmbox(path, mboxname);
>  
>  	mesgpat = regcomp("([0-9]+)(/.*)?");
>  	cwait = threadwaitchan();
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [9front] Mail rewrite; open path with stored mails
  2021-02-14 20:57           ` ori
@ 2021-02-14 22:05             ` theinicke
  0 siblings, 0 replies; 12+ messages in thread
From: theinicke @ 2021-02-14 22:05 UTC (permalink / raw)
  To: 9front

Thanks for the write-up. I appreciate it and get most of your points.
Here are some notes regarding your comments:

> Why not just make this flag modify the interpretation
> of argv[0]?

Frankly I do not get this - I thought about the new option behaving as opening the desired path at given name,
if name is given via argv[0] (otherwise using last path component as name).

> closembox()?

removeopened is arguably not a nice name, however closembox would be imho not so good either, because we only close previously opened mbox and not in general

> There's no real harm in skipping this special case.
> It's one open in code that does an open for every
> mail in the mailbox.

What special case exactly? I thought that we may want to be able to open relative paths (for that I used fd2path(2));
this however would remove us from having the possibility to open another imap (/imaps/mailserver/...) directory - thus if the path is already absolute I decided to leave it as it is (except for 1 possibly trailing slash, as this happens likely; with multiple trailing slashes one may be doomed, but deserves it probably).

> Why the termination and unterminaton?

Arguably ugly - still use abspath below (read: extract last path component and leave absolute path intact)..

> Why is this being repeated 10 times? [...]

It tries with a modified mbox name each time - say you got something like
$home/mail/9front/archive
and
$home/mail/9fans/archive
Then upon invoking 'Mail -p $home/mail/9front/archive' you would get /mail/fs/archive
and if you have this running somewhere and decided to open your other archive (without passing argument) this would be opened as /mail/fs/archive-0

That way we may have up to 11 mailboxes with the same name - 11 may be unnecessary high, but no harm I think? Taken from old Mail.


The remaining comments point out some obvious flaws and I'll gladly fix them; just wanted to quickly leave this here and thanks again for the feedback


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [9front] Mail rewrite; open path with stored mails
  2021-02-14 10:42         ` theinicke
  2021-02-14 20:57           ` ori
@ 2021-02-14 22:55           ` Alex Musolino
  2021-02-15  8:15             ` theinicke
  1 sibling, 1 reply; 12+ messages in thread
From: Alex Musolino @ 2021-02-14 22:55 UTC (permalink / raw)
  To: 9front

> +	if(atexit(removeopened) == 0) {
> +		removeopened();
> +		sysfatal("atexit: %r");
> +	}

This is not how atexit(2) works.  The idea is that you would do
atexit(removeopened) *once* in main which arranges for removeopened to
be called on the way out of the program.  The following should
suffice:

	if(atexit(removeopened) == 0)
		sysfatal("atexit: %r");


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [9front] Mail rewrite; open path with stored mails
  2021-02-14 22:55           ` Alex Musolino
@ 2021-02-15  8:15             ` theinicke
  2021-02-15  9:56               ` theinicke
  0 siblings, 1 reply; 12+ messages in thread
From: theinicke @ 2021-02-15  8:15 UTC (permalink / raw)
  To: 9front

Quoth Alex Musolino <alex@musolino.id.au>:
> > +	if(atexit(removeopened) == 0) {
> > +		removeopened();
> > +		sysfatal("atexit: %r");
> > +	}
> 
> This is not how atexit(2) works.  The idea is that you would do
> atexit(removeopened) *once* in main which arranges for removeopened to
> be called on the way out of the program.  The following should
> suffice:
> 
> 	if(atexit(removeopened) == 0)
> 		sysfatal("atexit: %r");
> 

atexit is only called once, because openmbox is only called once;
will move it into main right after openmbox to avoid potential future duplicate calls however, thanks.
I decided to call removeopened() if atexit fails,
because if we exit the program anyway then we may remove the mbox then and there?


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [9front] Mail rewrite; open path with stored mails
  2021-02-15  8:15             ` theinicke
@ 2021-02-15  9:56               ` theinicke
  0 siblings, 0 replies; 12+ messages in thread
From: theinicke @ 2021-02-15  9:56 UTC (permalink / raw)
  To: 9front

Ouch.. I should not write code and mails while being tired and doing other things...
I apologize to anyone who has wasted time reading through my previous patch.
@Ori: Thanks again for your feedback - and also your patience.
What follows is a patch that adresses most of the feedback and hopefully looks better:

diff -r 02e3059af5bc sys/man/1/acmemail
--- a/sys/man/1/acmemail	Thu Feb 11 09:37:36 2021 +0100
+++ b/sys/man/1/acmemail	Mon Feb 15 10:48:11 2021 +0100
@@ -1,4 +1,4 @@
-.TH NAIL 1
+.TH ACMEMAIL 1
 .SH NAME
 Mail \- view mail in acme
 
@@ -6,7 +6,7 @@
 
 Mail
 [
-.B -OsT
+.B -TOs
 ]
 [
 .B -m
@@ -20,6 +20,13 @@
 .B -o
 .I outbox
 ]
+[
+.B -p
+.I path
+]
+[
+.I mbox
+]
 
 .SH DESCRIPTION
 .PP
@@ -64,6 +71,10 @@
 Save a copy of outgoing messages to the mailbox
 .IR outbox ,
 instead of discarding them after they're enqueued.
+.TP
+.BI -p " path
+Opens new mailbox for the given path,
+closes it on exit.
 
 .PP
 Mail presents and acme interface for a upas/fs mailbox.
diff -r 02e3059af5bc sys/src/cmd/upas/Mail/mbox.c
--- a/sys/src/cmd/upas/Mail/mbox.c	Thu Feb 11 09:37:36 2021 +0100
+++ b/sys/src/cmd/upas/Mail/mbox.c	Mon Feb 15 10:48:11 2021 +0100
@@ -45,6 +45,31 @@
 
 static void	showmesg(Biobuf*, Mesg*, int, int);
 
+int
+openctlfile(void)
+{
+	int fd;
+	char *path;
+
+	path = estrjoin(maildir, "/ctl", nil);
+	fd = open(path, OWRITE);
+	free(path);
+	return fd;
+}
+
+void
+closembox(void)
+{
+	int fd;
+
+	fd = openctlfile();
+	if(fd == -1)
+		return;
+
+	fprint(fd, "close %s", mailbox);
+	close(fd);
+}
+
 static void
 plumbloop(Channel *ch, int fd)
 {
@@ -676,13 +701,10 @@
 mbflush(char **, int)
 {
 	int i, j, ln, fd;
-	char *path;
 	Mesg *m, *p;
 
 	i = 0;
-	path = estrjoin(maildir, "/ctl", nil);
-	fd = open(path, OWRITE);
-	free(path);
+	fd = openctlfile();
 	if(fd == -1)
 		sysfatal("open mbox: %r");
 	while(i < mbox.nmesg){
@@ -975,15 +997,91 @@
 static void
 usage(void)
 {
-	fprint(2, "usage: %s [-T] [-m mailfs] [-s] [-f format] [mbox]\n", argv0);
+	fprint(2, "usage: %s [-TOs] [-m maildir] [-f format] [-o outbox] [-p path] [mbox]\n", argv0);
 	exits("usage");
 }
 
+static char*
+basename(char *path)
+{
+	char *base;
+	base = strrchr(path, '/');
+	if(base == nil)
+		base = path;
+	return base+1;
+}
+
+static char*
+absmboxpath(char *path)
+{
+	char *abspath;
+	char buf[512];
+	int fd, i;
+
+	/* if path is already absolute, leave it as it is - to allow /imap...
+	otherwise get absolute path from relative one */
+	if(*path == '/'){
+		i = strlen(path);
+		while(i > 0 && path[--i] == '/')
+			path[i] = '\0';
+		abspath = estrdup(path);
+	}else{
+		fd = open(path, OREAD);
+		if(fd < 0)
+			sysfatal("can't open %s: %r", path);
+	
+		if(fd2path(fd, buf, sizeof buf) != 0)
+			sysfatal("fd2path %s: %r", path);
+		close(fd);
+		abspath = estrdup(buf);
+	}
+
+	return abspath;
+}
+
+static void
+openmbox(char *path, char *mboxname)
+{
+	int fd, i;
+	char err[ERRMAX];
+	char *name, *fsname, *abspath;
+
+	abspath = absmboxpath(path);
+
+	if(mboxname != nil)
+		name = mboxname;
+	else{
+		name = basename(abspath);
+	}
+
+	fd = openctlfile();
+	if(fd == -1)
+		sysfatal("open mbox: %r");
+	fsname = estrdup(name);
+	for(i=0; i<10; i++){
+		if(fprint(fd, "open %s %s", abspath, fsname) >= 0)
+			break;
+		err[0] = '\0';
+		errstr(err, sizeof err);
+		if(strstr(err, "mbox name in use") == nil)
+			sysfatal("can't create directory %s for mail: %s", name, err);
+		free(fsname);
+		fsname = smprint("%s-%d", name, i);
+	}
+	if(i == 10)
+		sysfatal("can't open %s: %r", abspath);
+
+	mailbox = fsname;
+	free(abspath);
+	close(fd);
+}
+
 void
 threadmain(int argc, char **argv)
 {
 	Fmt fmt;
 	char *cmd;
+	char *path = nil, *mboxname = nil;
 	int i;
 
 	mbox.view = Vgroup;
@@ -1017,6 +1115,9 @@
 	case 'o':
 		savebox = EARGF(usage());
 		break;
+	case 'p':
+		path = EARGF(usage());
+		break;
 	default:
 		usage();
 		break;
@@ -1024,8 +1125,18 @@
 
 	if(argc > 1)
 		usage();
-	if(argc == 1)
+	if(argc == 1) {
 		mailbox = argv[0];
+		mboxname = mailbox;
+	}
+
+	if(path != nil) {
+		openmbox(path, mboxname);
+		if(atexit(closembox) == 0) {
+			closembox();
+			sysfatal("atexit: %r");
+		}
+	}
 
 	mesgpat = regcomp("([0-9]+)(/.*)?");
 	cwait = threadwaitchan();



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [9front] Mail rewrite open path with stored mails
@ 2021-02-12 20:50 theinicke
  0 siblings, 0 replies; 12+ messages in thread
From: theinicke @ 2021-02-12 20:50 UTC (permalink / raw)
  To: 9front

I have noticed that Mail/Nail does no longer open additional mailboxes if passed a path [containing mails as undecoded MIME messages]. Now I am wondering whether this is by design or it is desirable to reintroduce this.

That said I can imagine some possibilities for reintroduction:

1) change argv parsing to interpret argument like old Mail did (e.g. mbox | path)
1a) also reintroduce second optional argument to open mailbox with given mboxname - imho unnecessary, but backwards compatibility?
2) introduce this via an additional parameter
3) just do it using a script

I could provide a patch implementing it, but I am not sure whether this was left out by design (e.g. because if one needed it one could easily do this with a script) and which solution to favor, thoughts?

--
Tobias Heinicke


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-02-15  9:59 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-12 20:54 [9front] Mail rewrite; open path with stored mails theinicke
2021-02-12 21:10 ` ori
2021-02-13 20:47   ` theinicke
2021-02-13 21:25     ` theinicke
2021-02-13 23:59       ` Alex Musolino
2021-02-14 10:42         ` theinicke
2021-02-14 20:57           ` ori
2021-02-14 22:05             ` theinicke
2021-02-14 22:55           ` Alex Musolino
2021-02-15  8:15             ` theinicke
2021-02-15  9:56               ` theinicke
  -- strict thread matches above, loose matches on Subject: below --
2021-02-12 20:50 [9front] Mail rewrite " theinicke

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).