Re: [9front] [patch] upas: plumb flag updates on messages.

Re: [9front] [patch] upas: plumb flag updates on messages.

[cinap_lenrek]

good you found it! no further issues on my side.

--
cinap

Re: [9front] [patch] upas: plumb flag updates on messages.

[ori]

>> ok, small remarks.
>> 
>> +			if(doplumb){
>> +				if((m->cstate & Cnew) && ensurecache(mb, m) == 0)
>>  					msgdecref(mb, m);
>> 
>> i think you want to do the msgdecref() of the message *after* mailplumb(),
>> not before. otherwise its like free before use. i know with the current
>> code it will still work but it is semantially wrong. if we later change
>> how cache eviction works that will blow up.
> 
> Ok. I was looking at the code, and it *seemed* like the intent was
> to have even evicted messages be somewhat valid. I think there are a
> number of places where we manipulate seen but not cached messages.
> 
>> +		print("child\n");
>> +		if(mboxfile != nil)
>> +			if(err = newmbox(mboxfile, "mbox", 0, nil))
>> +				sysfatal("opening %s: %s", mboxfile, err);
> 
>> theres a debug print left there and this changes some semantics.
>> i know this is because initial sync from imap takes forever. but
>> people sometimes want to specify mailbox path at the initial
>> command line and expect error status if the initial sync failed
>> (like if mounting a mbox file in a script to parse a message).
> 
> Right, forgot about this change. Reverted -- It doesn't even
> make things respon faster, since the mount doesn't return until
> after we do the initial sync.
> 
>> i think a good compromise would be to add a commandline flag to
>> specify if you want initial sync in the background or not
>> (can then be added to /rc/bin/startupasfs).
>>
>> if we do background syncing, then even if the initial sync
>> fails, it should try to continue. imagine your imap server
>> is currently down or network is unavailable, you want it to
>> continue trying in that case. and sysfataling here makes no
>> sense as nobody will be listening.
> 
> Yes. background sync will need a bunch more work, and a lot
> more thought to get right.
> 
> Updated patch below, slipped in 2 small changes:
> 
>  - Acme Mail => Next button to take you to the next msg
>  - Imap => small consistency tweak: flags are uint.
> 
> Can back them out if you care about separating them.
> 


+			if((m->cstate & Cnew|Cmod) && ensurecache(mb, m) == 0){

And I attached the wrong version of the patch. The one I tested
has (m->cstate & (Cnew|Cmod))

Re: [9front] [patch] upas: plumb flag updates on messages.

[cinap_lenrek]

ok, small remarks.

 char*
 syncmbox(Mailbox *mb, int doplumb)
 {
@@ -90,16 +87,14 @@
 				cachehash(mb, m);
 				m->cstate |= Cnew;
 				n++;
-			} else if(!doplumb)
-				m->cstate &= ~Cnew;
-			if(m->cstate & Cnew){
-				if(ensurecache(mb, m) == 0){
-					if(doplumb)
-						mailplumb(mb, m);
+			}
+			if(doplumb){
+				if((m->cstate & Cnew) && ensurecache(mb, m) == 0)
 					msgdecref(mb, m);
-				}
-				m->cstate &= ~Cnew;
+				if(m->cstate & (Cnew|Cmod))
+					mailplumb(mb, m);
 			}
+			m->cstate &= ~(Cnew|Cmod);
 		}
 		if(m->cstate & Cidxstale)

i think you want to do the msgdecref() of the message *after* mailplumb(),
not before. otherwise its like free before use. i know with the current
code it will still work but it is semantially wrong. if we later change
how cache eviction works that will blow up.

diff -r 7f3f8606fc12 sys/src/cmd/upas/fs/fs.c
--- a/sys/src/cmd/upas/fs/fs.c	Sun Dec 08 11:58:52 2019 -0800
+++ b/sys/src/cmd/upas/fs/fs.c	Sun Dec 08 11:59:39 2019 -0800
@@ -337,14 +337,15 @@
 		mboxfile = mbox;
 	}
 
-	if(mboxfile != nil)
-		if(err = newmbox(mboxfile, "mbox", 0, nil))
-			sysfatal("opening %s: %s", mboxfile, err);
 
 	switch(rfork(RFFDG|RFPROC|RFNAMEG|RFNOTEG|RFREND)){
 	case -1:
 		error("fork");
 	case 0:
+		print("child\n");
+		if(mboxfile != nil)
+			if(err = newmbox(mboxfile, "mbox", 0, nil))
+				sysfatal("opening %s: %s", mboxfile, err);
 		henter(PATH(0, Qtop), dirtab[Qctl],
 			(Qid){PATH(0, Qctl), 0, QTFILE}, nil, nil);
 		close(p[1]);

theres a debug print left there and this changes some semantics.
i know this is because initial sync from imap takes forever. but
people sometimes want to specify mailbox path at the initial
command line and expect error status if the initial sync failed
(like if mounting a mbox file in a script to parse a message).

i think a good compromise would be to add a commandline flag to
specify if you want initial sync in the background or not
(can then be added to /rc/bin/startupasfs).

if we do background syncing, then even if the initial sync
fails, it should try to continue. imagine your imap server
is currently down or network is unavailable, you want it to
continue trying in that case. and sysfataling here makes no
sense as nobody will be listening.

--
cinap

Re: [9front] [patch] upas: plumb flag updates on messages.

[ori]

> ok, small remarks.
> 
> +			if(doplumb){
> +				if((m->cstate & Cnew) && ensurecache(mb, m) == 0)
>  					msgdecref(mb, m);
> 
> i think you want to do the msgdecref() of the message *after* mailplumb(),
> not before. otherwise its like free before use. i know with the current
> code it will still work but it is semantially wrong. if we later change
> how cache eviction works that will blow up.

Ok. I was looking at the code, and it *seemed* like the intent was
to have even evicted messages be somewhat valid. I think there are a
number of places where we manipulate seen but not cached messages.

> +		print("child\n");
> +		if(mboxfile != nil)
> +			if(err = newmbox(mboxfile, "mbox", 0, nil))
> +				sysfatal("opening %s: %s", mboxfile, err);

> theres a debug print left there and this changes some semantics.
> i know this is because initial sync from imap takes forever. but
> people sometimes want to specify mailbox path at the initial
> command line and expect error status if the initial sync failed
> (like if mounting a mbox file in a script to parse a message).

Right, forgot about this change. Reverted -- It doesn't even
make things respon faster, since the mount doesn't return until
after we do the initial sync.

> i think a good compromise would be to add a commandline flag to
> specify if you want initial sync in the background or not
> (can then be added to /rc/bin/startupasfs).
>
> if we do background syncing, then even if the initial sync
> fails, it should try to continue. imagine your imap server
> is currently down or network is unavailable, you want it to
> continue trying in that case. and sysfataling here makes no
> sense as nobody will be listening.

Yes. background sync will need a bunch more work, and a lot
more thought to get right.

Updated patch below, slipped in 2 small changes:

 - Acme Mail => Next button to take you to the next msg
 - Imap => small consistency tweak: flags are uint.

Can back them out if you care about separating them.

diff -r eda8210ca3fc sys/src/cmd/faces/plumb.c
--- a/sys/src/cmd/faces/plumb.c	Mon Dec 09 02:03:10 2019 +0100
+++ b/sys/src/cmd/faces/plumb.c	Mon Dec 09 10:05:59 2019 -0800
@@ -273,13 +273,18 @@
 			if(m == nil)
 				killall("error on seemail plumb port");
 			t = value(m->attr, "mailtype", "");
-			if(strcmp(t, "delete") == 0)
+			if(strcmp(t, "modify") == 0)
+				goto Ignore;
+			else if(strcmp(t, "delete") == 0)
 				delete(m->data, value(m->attr, "digest", nil));
-			else if(strcmp(t, "new") != 0)
+			else if(strcmp(t, "new") == 0)
+				for(i=0; i<nmaildirs; i++){
+					if(strncmp(m->data, maildirs[i], strlen(maildirs[i])) == 0)
+						goto Found;
+				}
+			else
 				fprint(2, "faces: unknown plumb message type %s\n", t);
-			else for(i=0; i<nmaildirs; i++)
-				if(strncmp(m->data, maildirs[i], strlen(maildirs[i])) == 0)
-					goto Found;
+		Ignore:
 			plumbfree(m);
 			continue;
 
diff -r eda8210ca3fc sys/src/cmd/upas/Mail/dat.h
--- a/sys/src/cmd/upas/Mail/dat.h	Mon Dec 09 02:03:10 2019 +0100
+++ b/sys/src/cmd/upas/Mail/dat.h	Mon Dec 09 10:05:59 2019 -0800
@@ -124,6 +124,7 @@
 
 extern	int		mesgadd(Message*, char*, Dir*, char*);
 extern	void		mesgmenu(Window*, Message*);
+extern	void		mesgmenuselect(Window*, Message*);
 extern	void		mesgmenunew(Window*, Message*);
 extern	void		mesgmenureflag(Window*, Message*);
 extern	int		mesgopen(Message*, char*, char*, Message*, int, char*);
diff -r eda8210ca3fc sys/src/cmd/upas/Mail/mail.c
--- a/sys/src/cmd/upas/Mail/mail.c	Mon Dec 09 02:03:10 2019 +0100
+++ b/sys/src/cmd/upas/Mail/mail.c	Mon Dec 09 10:05:59 2019 -0800
@@ -189,7 +189,7 @@
 
 	wbox = newwindow();
 	winname(wbox, mbox.name);
-	wintagwrite(wbox, "Put Mail Delmesg Save ", 3+1+4+1+7+1+4+1);
+	wintagwrite(wbox, "Put Mail Delmesg Save Next ", 3+1+4+1+7+1+4+1+4+1);
 	threadcreate(mainctl, wbox, STACK);
 
 	fmtstrinit(&fmt);
@@ -317,6 +317,22 @@
 	}
 }
 
+void
+modmesg(char *name, char *digest)
+{
+	Message *m;
+	char *flags;
+
+	if((m = mesglookupfile(&mbox, name, digest)) == nil)
+		return;
+	if((flags = readfile(name, "/flags", nil)) == nil)
+		return;
+	free(m->flags);
+	m->flags = flags;
+	mesgmenureflag(mbox.w, m);
+}
+	
+
 extern int mesgsave(Message*, char*);
 void
 savemesg(char *box, char *name, char *digest)
@@ -361,6 +377,8 @@
 			newmesg(m->data, digest);
 		else if(strcmp(type, "delete") == 0)
 			delmesg(m->data, digest, 0);
+		else if(strcmp(type, "modify") == 0)
+			modmesg(m->data, digest);
 		else
 			fprint(2, "Mail: unknown plumb attribute %s\n", type);
 		plumbfree(m);
@@ -513,6 +531,11 @@
 		free(targs);
 		return 1;
 	}
+	if(strcmp(args[0], "Next") == 0){
+		ctlprint(w->ctl, "addr=dot\n");
+		winselect(w, "/^[0-9]*\\/ \\[\\*.*(\\n(\t.*)*)/", 1);
+		ctlprint(w->ctl, "show\n");
+	}
 	return 0;
 }
 
diff -r eda8210ca3fc sys/src/cmd/upas/fs/dat.h
--- a/sys/src/cmd/upas/fs/dat.h	Mon Dec 09 02:03:10 2019 +0100
+++ b/sys/src/cmd/upas/fs/dat.h	Mon Dec 09 10:05:59 2019 -0800
@@ -7,6 +7,7 @@
 	Cheader 	= 1<<2,
 	Cbody		= 1<<3,
 	Cnew		= 1<<4,
+	Cmod		= 1<<5,
 
 	/* encodings */
 	Enone=	0,
diff -r eda8210ca3fc sys/src/cmd/upas/fs/imap.c
--- a/sys/src/cmd/upas/fs/imap.c	Mon Dec 09 02:03:10 2019 +0100
+++ b/sys/src/cmd/upas/fs/imap.c	Mon Dec 09 10:05:59 2019 -0800
@@ -35,7 +35,7 @@
 	uvlong	uid;
 	ulong	sizes;
 	ulong	dates;
-	ulong	flags;
+	uint	flags;
 } Fetchi;
 
 typedef struct Imap Imap;
@@ -1006,6 +1006,8 @@
 			m->deleted = Disappear;
 			ll = &m->next;
 		}else{
+			if((m->flags & ~Frecent) != (f[i].flags & ~Frecent))
+				m->cstate |= Cmod;
 			m->flags = f[i].flags;
 			ll = &m->next;
 			i++;
diff -r eda8210ca3fc sys/src/cmd/upas/fs/mbox.c
--- a/sys/src/cmd/upas/fs/mbox.c	Mon Dec 09 02:03:10 2019 +0100
+++ b/sys/src/cmd/upas/fs/mbox.c	Mon Dec 09 10:05:59 2019 -0800
@@ -58,9 +58,6 @@
 static void delmessage(Mailbox*, Message*);
 static void mailplumb(Mailbox*, Message*);
 
-/*
- * do we want to plumb flag changes?
- */
 char*
 syncmbox(Mailbox *mb, int doplumb)
 {
@@ -90,16 +87,15 @@
 				cachehash(mb, m);
 				m->cstate |= Cnew;
 				n++;
-			} else if(!doplumb)
-				m->cstate &= ~Cnew;
-			if(m->cstate & Cnew){
-				if(ensurecache(mb, m) == 0){
-					if(doplumb)
+			}
+			if(doplumb){
+				if((m->cstate & Cnew) && ensurecache(mb, m) == 0){
+					if(m->cstate & Cmod)
 						mailplumb(mb, m);
 					msgdecref(mb, m);
 				}
-				m->cstate &= ~Cnew;
 			}
+			m->cstate &= ~(Cnew|Cmod);
 		}
 		if(m->cstate & Cidxstale)
 			y++;
@@ -1555,7 +1551,12 @@
 	a[ai-1].next = &a[ai];
 
 	a[++ai].name = "mailtype";
-	a[ai].value = !m->inmbox ? "delete": "new";
+	if(m->cstate & Cmod)
+		a[ai].value = "modify";
+	else if (!m->inmbox)
+		a[ai].value = "delete";
+	else
+		a[ai].value = "new";
 	a[ai-1].next = &a[ai];
 
 	snprint(date, sizeof date, "%Δ", m->fileid);

Re: [9front] [patch] upas: plumb flag updates on messages.

[ori]

>> ok, small remarks.
>> 
>> +			if(doplumb){
>> +				if((m->cstate & Cnew) && ensurecache(mb, m) == 0)
>>  					msgdecref(mb, m);
>> 
>> i think you want to do the msgdecref() of the message *after* mailplumb(),
>> not before. otherwise its like free before use. i know with the current
>> code it will still work but it is semantially wrong. if we later change
>> how cache eviction works that will blow up.
> 
> Ok. I was looking at the code, and it *seemed* like the intent was
> to have even evicted messages be somewhat valid. I think there are a
> number of places where we manipulate seen but not cached messages.
> 
>> +		print("child\n");
>> +		if(mboxfile != nil)
>> +			if(err = newmbox(mboxfile, "mbox", 0, nil))
>> +				sysfatal("opening %s: %s", mboxfile, err);
> 
>> theres a debug print left there and this changes some semantics.
>> i know this is because initial sync from imap takes forever. but
>> people sometimes want to specify mailbox path at the initial
>> command line and expect error status if the initial sync failed
>> (like if mounting a mbox file in a script to parse a message).
> 
> Right, forgot about this change. Reverted -- It doesn't even
> make things respon faster, since the mount doesn't return until
> after we do the initial sync.
> 
>> i think a good compromise would be to add a commandline flag to
>> specify if you want initial sync in the background or not
>> (can then be added to /rc/bin/startupasfs).
>>
>> if we do background syncing, then even if the initial sync
>> fails, it should try to continue. imagine your imap server
>> is currently down or network is unavailable, you want it to
>> continue trying in that case. and sysfataling here makes no
>> sense as nobody will be listening.
> 
> Yes. background sync will need a bunch more work, and a lot
> more thought to get right.
> 
> Updated patch below, slipped in 2 small changes:
> 
>  - Acme Mail => Next button to take you to the next msg
>  - Imap => small consistency tweak: flags are uint.
> 
> Can back them out if you care about separating them.
> 

One more revision, now without typoed redundant checks.

diff -r eda8210ca3fc sys/src/cmd/faces/plumb.c
--- a/sys/src/cmd/faces/plumb.c	Mon Dec 09 02:03:10 2019 +0100
+++ b/sys/src/cmd/faces/plumb.c	Mon Dec 09 10:27:56 2019 -0800
@@ -273,13 +273,18 @@
 			if(m == nil)
 				killall("error on seemail plumb port");
 			t = value(m->attr, "mailtype", "");
-			if(strcmp(t, "delete") == 0)
+			if(strcmp(t, "modify") == 0)
+				goto Ignore;
+			else if(strcmp(t, "delete") == 0)
 				delete(m->data, value(m->attr, "digest", nil));
-			else if(strcmp(t, "new") != 0)
+			else if(strcmp(t, "new") == 0)
+				for(i=0; i<nmaildirs; i++){
+					if(strncmp(m->data, maildirs[i], strlen(maildirs[i])) == 0)
+						goto Found;
+				}
+			else
 				fprint(2, "faces: unknown plumb message type %s\n", t);
-			else for(i=0; i<nmaildirs; i++)
-				if(strncmp(m->data, maildirs[i], strlen(maildirs[i])) == 0)
-					goto Found;
+		Ignore:
 			plumbfree(m);
 			continue;
 
diff -r eda8210ca3fc sys/src/cmd/upas/Mail/dat.h
--- a/sys/src/cmd/upas/Mail/dat.h	Mon Dec 09 02:03:10 2019 +0100
+++ b/sys/src/cmd/upas/Mail/dat.h	Mon Dec 09 10:27:56 2019 -0800
@@ -124,6 +124,7 @@
 
 extern	int		mesgadd(Message*, char*, Dir*, char*);
 extern	void		mesgmenu(Window*, Message*);
+extern	void		mesgmenuselect(Window*, Message*);
 extern	void		mesgmenunew(Window*, Message*);
 extern	void		mesgmenureflag(Window*, Message*);
 extern	int		mesgopen(Message*, char*, char*, Message*, int, char*);
diff -r eda8210ca3fc sys/src/cmd/upas/Mail/mail.c
--- a/sys/src/cmd/upas/Mail/mail.c	Mon Dec 09 02:03:10 2019 +0100
+++ b/sys/src/cmd/upas/Mail/mail.c	Mon Dec 09 10:27:56 2019 -0800
@@ -189,7 +189,7 @@
 
 	wbox = newwindow();
 	winname(wbox, mbox.name);
-	wintagwrite(wbox, "Put Mail Delmesg Save ", 3+1+4+1+7+1+4+1);
+	wintagwrite(wbox, "Put Mail Delmesg Save Next ", 3+1+4+1+7+1+4+1+4+1);
 	threadcreate(mainctl, wbox, STACK);
 
 	fmtstrinit(&fmt);
@@ -317,6 +317,22 @@
 	}
 }
 
+void
+modmesg(char *name, char *digest)
+{
+	Message *m;
+	char *flags;
+
+	if((m = mesglookupfile(&mbox, name, digest)) == nil)
+		return;
+	if((flags = readfile(name, "/flags", nil)) == nil)
+		return;
+	free(m->flags);
+	m->flags = flags;
+	mesgmenureflag(mbox.w, m);
+}
+	
+
 extern int mesgsave(Message*, char*);
 void
 savemesg(char *box, char *name, char *digest)
@@ -361,6 +377,8 @@
 			newmesg(m->data, digest);
 		else if(strcmp(type, "delete") == 0)
 			delmesg(m->data, digest, 0);
+		else if(strcmp(type, "modify") == 0)
+			modmesg(m->data, digest);
 		else
 			fprint(2, "Mail: unknown plumb attribute %s\n", type);
 		plumbfree(m);
@@ -513,6 +531,11 @@
 		free(targs);
 		return 1;
 	}
+	if(strcmp(args[0], "Next") == 0){
+		ctlprint(w->ctl, "addr=dot\n");
+		winselect(w, "/^[0-9]*\\/ \\[\\*.*(\\n(\t.*)*)/", 1);
+		ctlprint(w->ctl, "show\n");
+	}
 	return 0;
 }
 
diff -r eda8210ca3fc sys/src/cmd/upas/fs/dat.h
--- a/sys/src/cmd/upas/fs/dat.h	Mon Dec 09 02:03:10 2019 +0100
+++ b/sys/src/cmd/upas/fs/dat.h	Mon Dec 09 10:27:56 2019 -0800
@@ -7,6 +7,7 @@
 	Cheader 	= 1<<2,
 	Cbody		= 1<<3,
 	Cnew		= 1<<4,
+	Cmod		= 1<<5,
 
 	/* encodings */
 	Enone=	0,
diff -r eda8210ca3fc sys/src/cmd/upas/fs/imap.c
--- a/sys/src/cmd/upas/fs/imap.c	Mon Dec 09 02:03:10 2019 +0100
+++ b/sys/src/cmd/upas/fs/imap.c	Mon Dec 09 10:27:56 2019 -0800
@@ -35,7 +35,7 @@
 	uvlong	uid;
 	ulong	sizes;
 	ulong	dates;
-	ulong	flags;
+	uint	flags;
 } Fetchi;
 
 typedef struct Imap Imap;
@@ -1006,6 +1006,8 @@
 			m->deleted = Disappear;
 			ll = &m->next;
 		}else{
+			if((m->flags & ~Frecent) != (f[i].flags & ~Frecent))
+				m->cstate |= Cmod;
 			m->flags = f[i].flags;
 			ll = &m->next;
 			i++;
diff -r eda8210ca3fc sys/src/cmd/upas/fs/mbox.c
--- a/sys/src/cmd/upas/fs/mbox.c	Mon Dec 09 02:03:10 2019 +0100
+++ b/sys/src/cmd/upas/fs/mbox.c	Mon Dec 09 10:27:56 2019 -0800
@@ -58,9 +58,6 @@
 static void delmessage(Mailbox*, Message*);
 static void mailplumb(Mailbox*, Message*);
 
-/*
- * do we want to plumb flag changes?
- */
 char*
 syncmbox(Mailbox *mb, int doplumb)
 {
@@ -90,16 +87,13 @@
 				cachehash(mb, m);
 				m->cstate |= Cnew;
 				n++;
-			} else if(!doplumb)
-				m->cstate &= ~Cnew;
-			if(m->cstate & Cnew){
-				if(ensurecache(mb, m) == 0){
-					if(doplumb)
-						mailplumb(mb, m);
-					msgdecref(mb, m);
-				}
-				m->cstate &= ~Cnew;
 			}
+			if((m->cstate & Cnew|Cmod) && ensurecache(mb, m) == 0){
+				if(doplumb)
+					mailplumb(mb, m);
+				msgdecref(mb, m);
+			}
+			m->cstate &= ~(Cnew|Cmod);
 		}
 		if(m->cstate & Cidxstale)
 			y++;
@@ -1555,7 +1549,12 @@
 	a[ai-1].next = &a[ai];
 
 	a[++ai].name = "mailtype";
-	a[ai].value = !m->inmbox ? "delete": "new";
+	if(m->cstate & Cmod)
+		a[ai].value = "modify";
+	else if (!m->inmbox)
+		a[ai].value = "delete";
+	else
+		a[ai].value = "new";
 	a[ai-1].next = &a[ai];
 
 	snprint(date, sizeof date, "%Δ", m->fileid);