Re: [9front] Mail rewrite; open path with stored mails

[ori@eigenstate.org]

> 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();
>