Hi
[2020-03-22 13:02] Philipp Takacs <phil...@bureaucracy.de>
> [2020-03-21 23:15] markus schnalke <mei...@marmaro.de>
> > [2020-03-21 18:43] Philipp Takacs <phil...@bureaucracy.de>
> > > [2020-03-21 10:05] markus schnalke <mei...@marmaro.de>
> > > > [2020-03-19 12:33] Philipp Takacs <phil...@bureaucracy.de>
> > > > > I have also removed the
> > > > > clear_msg_flags loop from folder_read and folder_realloc. In most
> > > > > cases
> > > > > we use calloc, so this isn't necesarry. At one place in folder_realloc
> > > > > realloc is used. There I have add a memset.
> > > > >
> > > > > comment?
> > > >
> > > > Why were (in the section down below) only the flags outside the
> > > > current message range cleared? Now you clear all flags.
> > >
> > > Not sure whats make you think this. But if I had to guess I would
> > > say it's the "- lo".
> >
> > No, it's the comment a bit more down in the code:
> >
> > > > - /*
> > > > - ** Clear all the flags for entries outside
> > > > - ** the current message range for this folder.
> > > > - */
>
> Now I get it.
>
> > Maybe I was irritated that you removed this comment but still
> > performed a similar action. The memset() line is pretty complex
> > to understand. You could add a similar two-line comment to it,
> > explaining what you explained to me here (only a bit more
> > destilled):
>
> Working on it.
>
> > > > Or, if your memset() clears only specific
> > > > parts, you probably should put a comment there to explain it, as
> > > > this might not be what the programmer usually expects from a
> > > > memset() after a (re)alloc().
> > >
> > > To me this looks completly like this what I would expect.
> >
> > Of course; you've written the code. ;-)
>
> I mean more a general view. So after a realloc I would expect something
> like:
>
> memset(ptr + oldsize + 1, 0, newsize - oldsize)
>
> of course my code dose calculate the oldsize a bit strange, but in it's
> more or less the same.
>
> > > But I'll think about a good comment.
> >
> > Is what you actually clear only the newly allocated memory? If so
> > then you could write something like:
> >
> > /* zero the newly allocated memory, i.e. no flags set */
> >
> > Or:
> >
> > /* clear the newly allocated msg flag space */
>
> Thanks for the hints. Looks now like this:
A updated version of the patches is attached. I'll commit them in the
next days.
Philipp
From 1d3f9f775569c4d3db54ca2a8670960dca68c445 Mon Sep 17 00:00:00 2001
From: Philipp Takacs <phil...@bureaucracy.de>
Date: Wed, 18 Mar 2020 19:53:44 +0100
Subject: [PATCH 1/3] set SEQMOD if clear_msg_flags change the flags
This fixes a bug in folder_addmsg. If the sequences file containes
the new msgnum, seq_save won't override this.
---
sbr/seq_msgstats.c | 2 ++
test/tests/rcv/test-rcvstore-nounseen | 18 ++++++++++++++++++
2 files changed, 20 insertions(+)
create mode 100755 test/tests/rcv/test-rcvstore-nounseen
diff --git a/sbr/seq_msgstats.c b/sbr/seq_msgstats.c
index 7ffbde16..bfd7fb27 100644
--- a/sbr/seq_msgstats.c
+++ b/sbr/seq_msgstats.c
@@ -27,6 +27,8 @@ void
clear_msg_flags(struct msgs *mp, int msgnum)
{
assert_msg_range(mp, msgnum);
+ if (mp->msgstats[msgnum - mp->lowoff])
+ mp->msgflags |= SEQMOD;
mp->msgstats[msgnum - mp->lowoff] = 0;
}
diff --git a/test/tests/rcv/test-rcvstore-nounseen b/test/tests/rcv/test-rcvstore-nounseen
new file mode 100755
index 00000000..b97f8edc
--- /dev/null
+++ b/test/tests/rcv/test-rcvstore-nounseen
@@ -0,0 +1,18 @@
+#!/bin/sh
+######################################################
+#
+# Test rcvstore
+#
+######################################################
+
+. "$MH_TEST_COMMON"
+
+# check -nounseen
+printf "u: %s\n" $(basename $(mhpath b)) >> $MH_TEST_DIR/Mail/inbox/.mh_sequences
+runandcheck "rcvstore -nounseen <$MH_TEST_DIR/Mail/inbox/1" <<!
+!
+runandcheck 'mark -sequence u -list' <<!
+u:
+!
+runandcheck 'diff -u "`mhpath f`" "`mhpath l`"' <<!
+!
--
2.20.1
From 1c7b18ff336dbdc9bd0f62775e18dc5851fd7316 Mon Sep 17 00:00:00 2001
From: Philipp Takacs <phil...@bureaucracy.de>
Date: Wed, 18 Mar 2020 20:02:19 +0100
Subject: [PATCH 2/3] don't manuall clear the message flags
because we use calloc the message flags are cleared
after the allocation.
---
sbr/folder_read.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/sbr/folder_read.c b/sbr/folder_read.c
index 722ee2fd..011958f8 100644
--- a/sbr/folder_read.c
+++ b/sbr/folder_read.c
@@ -130,14 +130,6 @@ folder_read(char *name)
mp->msgstats = mh_xcalloc(MSGSTATSIZE(mp, mp->lowoff, mp->hghoff), 1);
- /*
- ** Clear all the flag bits for all the message
- ** status entries we just allocated.
- ** TODO: use memset() ?
- */
- for (msgnum = mp->lowoff; msgnum <= mp->hghoff; msgnum++)
- clear_msg_flags(mp, msgnum);
-
/*
** Scan through the array of messages we've seen and
** setup the initial flags for those messages in the
--
2.20.1
From 203db0876877d4f25bd4707fc8b2db7cce4d019d Mon Sep 17 00:00:00 2001
From: Philipp Takacs <phil...@bureaucracy.de>
Date: Wed, 18 Mar 2020 21:46:30 +0100
Subject: [PATCH 3/3] use memset to clear the msgstats in folder_realloc
---
sbr/folder_realloc.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/sbr/folder_realloc.c b/sbr/folder_realloc.c
index 64a66409..52c3647d 100644
--- a/sbr/folder_realloc.c
+++ b/sbr/folder_realloc.c
@@ -46,6 +46,12 @@ folder_realloc(struct msgs *mp, int lo, int hi)
** just realloc the message status array.
*/
mp->msgstats = mh_xrealloc(mp->msgstats, MSGSTATSIZE(mp, lo, hi));
+ /*
+ ** Clear the newly allocated msg flag space. The lowoff and
+ ** hghoff are the old messagenumber range. So the calculation
+ ** of the first new element has to subtract lowoff.
+ */
+ memset(mp->msgstats + mp->hghoff - lo + 1, 0, hi - mp->hghoff);
} else {
/*
** We are changing the offset of the message status
@@ -69,20 +75,5 @@ folder_realloc(struct msgs *mp, int lo, int hi)
mp->lowoff = lo;
mp->hghoff = hi;
- /*
- ** Clear all the flags for entries outside
- ** the current message range for this folder.
- */
- if (mp->nummsg > 0) {
- for (msgnum = mp->lowoff; msgnum < mp->lowmsg; msgnum++)
- clear_msg_flags(mp, msgnum);
- for (msgnum = mp->hghmsg + 1; msgnum <= mp->hghoff; msgnum++)
- clear_msg_flags(mp, msgnum);
- } else {
- /* no messages, so clear entire range */
- for (msgnum = mp->lowoff; msgnum <= mp->hghoff; msgnum++)
- clear_msg_flags(mp, msgnum);
- }
-
return mp;
}
--
2.20.1
