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