isync-devel — All-purpose list, actually - but RTFM first. Moderated for non-subscribers for SPAM protection.

Re: maildir_find_new_msgs: Assertion `DFlags & FAKEDUMBSTORE' failed.

[Piotr M. <pi...@ma...>]

Hello list,

I am another person who has encountered this bug.  I also have some new
findings.  E.g. that this assertion can be hit even on v1.5.0!

I wanted to do a git bisect, but the build process fails if I specify a
checkout instead of a release tarball (even for v1.5.1).  It's probably
an issue with how isync is packaged by GNU Guix, but I don't really know
how to debug it.  And I don't have the time right now to set up a build
environment manually, the traditional way.

Moreover, I checked that applying a revert of the commit 1e0b661d on top
of v1.5.1 restores the v1.5.0 behaviour.  This narrows down the recent
increased prevalence of this bug to that tiny change.

Also, here's an anonymized version of the rc file I was using:

#+BEGIN_EXAMPLE
IMAPAccount [...]
Host [...]
User [...]
PassCmd "pass show [...] | head -n1"
TLSType IMAPS
CertificateFile /etc/ssl/certs/ca-certificates.crt

IMAPStore remote
Account [...]

MaildirStore local
SubFolders Verbatim
Path ~/[...]/maildir/
Inbox ~/[...]/maildir/local-inbox

Channel synchronizer
Far :remote:
Near :local:
Sync Pull
Create Near
Remove None
Expunge None
ExpungeSolo None
CopyArrivalDate yes
SyncState *
#+END_EXAMPLE

* an already-downloaded maildir prevents the crash

#+BEGIN_EXAMPLE
me@host$ guix shell isync --with-source=isync@1.5.1=isync-1.5.1.tar.gz
me@guix$ rm -r maildir/*
[...]
removed directory 'maildir/local-inbox'
me@guix$ mbsync --config rc-file synchronizer
Maildir notice: no UIDVALIDITY in $PWD/maildir/local-inbox, creating new.
Channels: 1    Boxes: 1    Far: +0 *0 #0 -0    Near: +440 *0 #0 -0
me@guix$ mbsync --config rc-file --dry-run synchronizer
Channels: 1    Boxes: 1    Far: +0 *0 #0 -0    Near: +0 *0 #0 -0
#+END_EXAMPLE

* silent in v1.5.0; present in v1.5.1

#+BEGIN_EXAMPLE
me@host$ guix shell isync --with-source=isync@1.5.0=isync-1.5.0.tar.gz
me@guix$ rm -r maildir/*
[...]
removed directory 'maildir/local-inbox'
me@guix$ mbsync --config rc-file --dry-run synchronizer
Maildir notice: no UIDVALIDITY in $PWD/maildir/local-inbox, creating new.
Processed 1 box(es) in 1 channel(s),
would pull 440 new message(s) and 0 flag update(s).
#+END_EXAMPLE

(this was not in fact a dry run – it populated the maildir with a lot of files)

#+BEGIN_EXAMPLE
me@host$ guix shell isync --with-source=isync@1.5.1=isync-1.5.1.tar.gz
me@guix$ rm -r maildir/*
[...]
removed directory 'maildir/local-inbox'
me@guix$ mbsync --config rc-file --dry-run synchronizer
Maildir notice: no UIDVALIDITY in $PWD/maildir/local-inbox, creating new.
mbsync: drv_maildir.c:1680: maildir_find_new_msgs: Assertion `DFlags & FAKEDUMBSTORE' failed.
zsh: IOT instruction (core dumped)  mbsync --config rc-file --dry-run synchronizer
#+END_EXAMPLE

* v1.5.1 + revert seems to "fix" it

#+BEGIN_EXAMPLE
me@host$ guix shell isync --with-patch=isync@1.5.1=./revert.diff
me@guix$ rm -r maildir/*
[...]
removed directory 'maildir/local-inbox'
me@guix$ mbsync --config rc-file --dry-run synchronizer
Maildir notice: no UIDVALIDITY in $PWD/maildir/local-inbox, creating new.
Channels: 1    Boxes: 1    Far: +0 *0 #0 -0    Near: +440 *0 #0 -0
#+END_EXAMPLE

(unsurprisingly, this is not a dry run – it populates the maildir; it doesn't crash)

* v1.5.1 + revert + =--debug= uncovers it again

#+BEGIN_EXAMPLE
me@host$ guix shell isync --with-patch=isync@1.5.1=./revert.diff
me@guix$ rm -r maildir/*
[...]
removed directory 'maildir/local-inbox'
me@guix$ mbsync --config rc-file --dry-run --debug synchronizer 1>crash-huh.stdout 2>crash-huh.stderr
zsh: IOT instruction (core dumped)  mbsync --config rc-file --dry-run --debug synchronizer > crash-huh.stdout 2>
#+END_EXAMPLE

(+seems to be a dry run;+ it crashes and does not populate the maildir)

* v1.5.0 is also affected

#+BEGIN_EXAMPLE
me@host$ --with-source=isync@1.5.0=isync-1.5.0.tar.gz
me@guix$ rm -r maildir/*
[...]
removed directory 'maildir/local-inbox'
me@guix$ mbsync --config rc-file --dry-run --debug synchronizer 1>crash-huh-too.stdout 2>crash-huh-too.stderr
zsh: IOT instruction (core dumped)  mbsync --config rc-file --dry-run --debug synchronizer > crash-huh-too.stdout
#+END_EXAMPLE

(just like above)

** diff between =crash-huh.*= and =crash-huh-too.*= – not much to see here

#+BEGIN_EXAMPLE
--- crash-huh.stderr    2026-05-01 22:46:31.738887271 +0200
+++ crash-huh-too.stderr        2026-05-02 00:19:32.139762496 +0200
@@ -1 +1 @@
-mbsync: drv_maildir.c:1680: maildir_find_new_msgs: Assertion `DFlags & FAKEDUMBSTORE' failed.
+mbsync: drv_maildir.c:1674: maildir_find_new_msgs: Assertion `DFlags & FAKEDUMBSTORE' failed.
#+END_EXAMPLE

#+BEGIN_EXAMPLE
--- crash-huh.stdout    2026-05-01 22:46:31.738887271 +0200
+++ crash-huh-too.stdout        2026-05-02 00:19:32.139762496 +0200
@@ -1,4 +1,4 @@
-isync 1.5.1 called with: '--config' 'rc-file' '--dry-run' '--debug' 'synchronizer'
+isync 1.5.0 called with: '--config' 'rc-file' '--dry-run' '--debug' 'synchronizer'
 Reading configuration file ./rc-file
 merge ops (in Channel 'synchronizer'):
   common: XOP_PULL
@@ -47,7 +47,7 @@
 Opening near side box INBOX...
 N: [ 4] Enter open_box
 Maildir notice: no UIDVALIDITY in $PWD/maildir/local-inbox, creating new.
-N: [ 4] Callback enter open_box, sts=0, uidvalidity=1777668391
+N: [ 4] Callback enter open_box, sts=0, uidvalidity=1777673971
 N: [ 4] Callback leave open_box
 N: [ 4] Leave open_box
 F: [ 1] Callback leave connect_store
@@ -963,7 +963,7 @@
   uid=4540  flags=S    size=0      tuid=?
 far side: 440 messages, 0 recent
 matching messages on far side against sync records
--> log: | 1 1777668391 (new UIDVALIDITYs)
+-> log: | 1 1777673971 (new UIDVALIDITYs)
 Synchronizing...
 F: Called get_supported_flags, ret=0xff
 N: Called get_supported_flags, ret=0xff
@@ -1414,446 +1414,446 @@
 propagating new messages
 N: Called get_uidnext, ret=0
 -> log: F 1 0 (save UIDNEXT of near side)
[...]
 N: Called get_memory_usage, ret=0
 F: [ 7] Enter fetch_msg [DRY], uid=257, want_flags=no, want_date=yes
 F: [ 7] Callback enter fetch_msg, sts=0, flags=, date=-1, size=0
#+END_EXAMPLE

* doesn't happen unless I use =--dry-run=

#+BEGIN_EXAMPLE
me@host$ guix shell isync --with-source=isync@1.5.1=isync-1.5.1.tar.gz
me@guix$ rm -r maildir/*
[...]
removed directory 'maildir/local-inbox'
me@guix$ mbsync --config rc-file synchronizer
Maildir notice: no UIDVALIDITY in $PWD/maildir/local-inbox, creating new.
Channels: 1    Boxes: 1    Far: +0 *0 #0 -0    Near: +440 *0 #0 -0
#+END_EXAMPLE

Curiously, this seemed to be much slower than the buggy not-so-dry runs.
Either this actually does more work or maybe the server is starting to
rate-limit me.

* here's how the =crash.std*= files were gathered

#+BEGIN_EXAMPLE
me@host$ guix shell isync --with-source=isync@1.5.1=isync-1.5.1.tar.gz
me@guix$ rm -r maildir/*
[...]
removed directory 'maildir/local-inbox'
me@guix$ mbsync --config rc-file --dry-run --debug synchronizer 1>crash.stdout 2>crash.stderr
zsh: IOT instruction (core dumped)  mbsync --config rc-file --dry-run --debug synchronizer > crash.stdout 2>
#+END_EXAMPLE

If anyone has an idea how to obtain =--debug= logs *without the crash*
that differ the least for these, I'm all ears.

Cheers,
Piotr Masłowski

[commit] master: Suppress unavoidable memrchr() warning

[ossi <os...@us...>]

commit 0949519b8199db9b70e1c56111893096aa6df1d5
Author:     Seth McDonald <de...@se...>
AuthorDate: Fri May 1 12:41:53 2026 +1000
Commit:     Oswald Buddenhagen <os...@us...>
CommitDate: Fri May 1 11:39:11 2026 +0200

    Suppress unavoidable memrchr() warning
    
    For consistency with glibc, our memrchr() implementation returns a
    non-const pointer derived from a const pointer passed as an argument.
    This can trigger the -Wcast-qual compiler warning, which we have
    enabled. So locally suppress it for the relevant cast.
    While at it, we also delay the cast to the last possible moment, which
    is the return statement. This is safer, and also consistent with glibc.
    
    Also use our uchar typedef rather than the nonstandard u_char, fixing
    the omission from commit 42cedc8f8.

 src/util.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/util.c b/src/util.c
index c110188..22e639f 100644
--- a/src/util.c
+++ b/src/util.c
@@ -454,12 +454,18 @@ vasprintf( char **strp, const char *fmt, va_list ap )
 void *
 memrchr( const void *s, int c, size_t n )
 {
-	u_char *b = (u_char *)s, *e = b + n;
+	const uchar *b = s;
+	const uchar *e = b + n;
 
-	while (--e >= b)
-		if (*e == c)
+	while (--e >= b) {
+		if (*e == c) {
+DIAG_PUSH
+DIAG_DISABLE("-Wcast-qual")
 			return (void *)e;
-	return 0;
+DIAG_POP
+		}
+	}
+	return NULL;
 }
 #endif
 

[PATCH v3] Suppress unavoidable memrchr() warning

[Seth M. <de...@se...>]

For consistency with glibc, our memrchr() implementation returns a
non-const pointer derived from a const pointer passed as an argument.
This can trigger the -Wcast-qual compiler warning, which we have
enabled.  So locally supress it for the relevant cast.

Also use the uchar typedef rather than the nonstandard u_char, fixing
the omission from commit 42cedc8f81c9.
---
 src/util.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/util.c b/src/util.c
index c110188d4fac..22e639f57d68 100644
--- a/src/util.c
+++ b/src/util.c
@@ -454,12 +454,18 @@ vasprintf( char **strp, const char *fmt, va_list ap )
 void *
 memrchr( const void *s, int c, size_t n )
 {
-	u_char *b = (u_char *)s, *e = b + n;
+	const uchar *b = s;
+	const uchar *e = b + n;
 
-	while (--e >= b)
-		if (*e == c)
+	while (--e >= b) {
+		if (*e == c) {
+DIAG_PUSH
+DIAG_DISABLE("-Wcast-qual")
 			return (void *)e;
-	return 0;
+DIAG_POP
+		}
+	}
+	return NULL;
 }
 #endif
 

base-commit: 9aaac66286910f547ec3068d3fd72afb4fe716bf
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2] Suppress unavoidable memrchr() warning

[Oswald B. <osw...@gm...>]

On Wed, Apr 29, 2026 at 09:28:56AM +1000, Seth McDonald wrote:
>Regarding the use of unions vs pragmas here, it seems the preferred
>method of suppressing the warning is still via pragmas, so I'll continue
>their use.
>
that's fine.

>Regarding the problematic cast, it may be better to perform it on return
>rather than immediately.  That is, retaining the const qualifier until
>the return statement, at which point we remove it.  This can add a
>little more safety, as it ensures the value isn't modified within the
>function.  Any thoughts?
>
indeed, both glibc and musl do it like that.

notably, neither tries to suppress the warning. presumably, they are 
both compiled without it.

>And while we're modifying the local variables, I'd like to change their
>type from u_char* to either unsigned char* or uint8_t*.  If a typedef
>must be used, I see no reason to use the nonstandard u_char when there's
>the equivalent C-standard uint8_t.
>
i prefer my own "uchar" type from the top of common.h (which could be 
plausibly inspired by qt). it's way less visually noisy than the 
standard type and the long form.
this particular instance is simply an omission from 42cedc8f.

Re: [PATCH v2] Suppress unavoidable memrchr() warning

[Seth M. <de...@se...>]

Hello all,

Apologies for the delay!  Been out studying and touching grass.

Regarding the use of unions vs pragmas here, it seems the preferred
method of suppressing the warning is still via pragmas, so I'll continue
their use.  Let me know if anyone still desires the union approach.

Regarding the problematic cast, it may be better to perform it on return
rather than immediately.  That is, retaining the const qualifier until
the return statement, at which point we remove it.  This can add a
little more safety, as it ensures the value isn't modified within the
function.  Any thoughts?

And while we're modifying the local variables, I'd like to change their
type from u_char* to either unsigned char* or uint8_t*.  If a typedef
must be used, I see no reason to use the nonstandard u_char when there's
the equivalent C-standard uint8_t.

Take care,
	Seth McDonald.

-- 
E9D1 26A5 F0D4 9DF7 792B  C2E2 B4BF 4530 D39B 2D51

Re: [PATCH v2] Suppress unavoidable memrchr() warning

[Oswald B. <osw...@gm...>]

On Fri, Apr 17, 2026 at 12:38:31AM +0300, Evgeniy Berdnikov wrote:
> I can't imagine compiler generating different memory addresses for 
> such members.
>
that's not the issue.
if the compiler decides that it's undefined behavior, it might do 
literally anything - usually it will just drop possibly large chunks of 
code. this has been increasingly happening over the last years as 
compilers are optimizing more and more aggressively. the "i can't 
imagine a compiler would do something so obviously developer-hostile" 
argument is wishful thinking.

Re: [PATCH v2] Suppress unavoidable memrchr() warning

[Evgeniy B. <bi...@pr...>]

  Hi, Michal.
  
On Thu, Apr 16, 2026 at 03:16:26PM +0200, Michal SuchАnek wrote:
> On Thu, Apr 16, 2026 at 08:03:30PM +1000, Seth McDonald wrote:
> > | A pointer to void shall have the same representation and alignment
> > | requirements as a pointer to a character type.  Similarly, pointers to
> > | qualified or unqualified versions of compatible types shall have the
> > | same representation and alignment requirements.
> > 
> > So the conversion from const void* to void*, and from void* to char*, is
> > not undefined behaviour.  As such, if char* was used instead of u_char*
> > (which I'm not entirely sure why it isn't already), Evgeniy's workaround
> > would be perfectly conformant to the C standard.
> > 
> > It also doesn't break strict aliasing; unions are explicitly allowed to
> > facilitate interpreting the same memory with incompatible types.
> 
> The problem is discussed at length eg. here:
> https://sqlpey.com/c/c-union-type-punning-behavior-c99-vs-c11-standards-explained/#c-union-type-punning-behavior-c99-vs-c11-standards-explained
> 
> In conclusion there is no explicit allowance, and obtuse compiler
> writers will intepret different union members as unrelated, and point to
> the standard to support such obtuse implementation.

 Despite of absence of explicit allowance, some common sense may be used,
 particularly for situation where union members of primary type have
 the same length and alignment. I can't imagine compiler generating
 different memory addresses for such members.

 It seems me the only problem is alignment of members within union.
 The article cited does not explain why one method is considered "safer"
 then another, and why footnote did not become a part of normative text.
 Maybe there are some additional reasons in addition to alignment, it would
 be nice to know. In any case, thanks for valuable remark.
-- 
 Eugene Berdnikov

Re: [PATCH v2] Suppress unavoidable memrchr() warning

[Michal S. <msu...@su...>]

On Thu, Apr 16, 2026 at 08:03:30PM +1000, Seth McDonald wrote:
> Hi,
> 
> On Thu, 16 Apr 2026 at 11:40:36 +0200, Michal Suchánek wrote:
> > On Thu, Apr 16, 2026 at 11:45:47AM +0300, Evgeniy Berdnikov wrote:
> > >         union { u_char *b; const void *cvoid_b; } ub;
> > > 
> > >         ub.cvoid_b = s;
> > >         u_char *e = ub.b + n;
> > >         while (--e >= ub.b)
> > >                 if (*e == c)
> > >         ...
> > > 
> > >  It assumes that pointers to all bare types are represented identically,
> > >  AFAIK, this is true for all existent C implementations.
> > 
> > Also it breaks the aliasing rules that C fabricates regardless of the
> > actual use of the data.
> 
> Actually in this specific case, it's (almost) fully defined within the C
> standard.  From the C11 spec (the latest I have access to is C11):
> 
> | A pointer to void shall have the same representation and alignment
> | requirements as a pointer to a character type.  Similarly, pointers to
> | qualified or unqualified versions of compatible types shall have the
> | same representation and alignment requirements.
> 
> So the conversion from const void* to void*, and from void* to char*, is
> not undefined behaviour.  As such, if char* was used instead of u_char*
> (which I'm not entirely sure why it isn't already), Evgeniy's workaround
> would be perfectly conformant to the C standard.
> 
> It also doesn't break strict aliasing; unions are explicitly allowed to
> facilitate interpreting the same memory with incompatible types.

The problem is discussed at length eg. here:
https://sqlpey.com/c/c-union-type-punning-behavior-c99-vs-c11-standards-explained/#c-union-type-punning-behavior-c99-vs-c11-standards-explained

In conclusion there is no explicit allowance, and obtuse compiler
writers will intepret different union members as unrelated, and point to
the standard to support such obtuse implementation.

Thanks

Michal

Re: [PATCH v2] Suppress unavoidable memrchr() warning

[Seth M. <de...@se...>]

Hi,

On Thu, 16 Apr 2026 at 11:40:36 +0200, Michal Suchánek wrote:
> On Thu, Apr 16, 2026 at 11:45:47AM +0300, Evgeniy Berdnikov wrote:
> >         union { u_char *b; const void *cvoid_b; } ub;
> > 
> >         ub.cvoid_b = s;
> >         u_char *e = ub.b + n;
> >         while (--e >= ub.b)
> >                 if (*e == c)
> >         ...
> > 
> >  It assumes that pointers to all bare types are represented identically,
> >  AFAIK, this is true for all existent C implementations.
> 
> Also it breaks the aliasing rules that C fabricates regardless of the
> actual use of the data.

Actually in this specific case, it's (almost) fully defined within the C
standard.  From the C11 spec (the latest I have access to is C11):

| A pointer to void shall have the same representation and alignment
| requirements as a pointer to a character type.  Similarly, pointers to
| qualified or unqualified versions of compatible types shall have the
| same representation and alignment requirements.

So the conversion from const void* to void*, and from void* to char*, is
not undefined behaviour.  As such, if char* was used instead of u_char*
(which I'm not entirely sure why it isn't already), Evgeniy's workaround
would be perfectly conformant to the C standard.

It also doesn't break strict aliasing; unions are explicitly allowed to
facilitate interpreting the same memory with incompatible types.

Take care,
	Seth McDonald.

-- 
E9D1 26A5 F0D4 9DF7 792B  C2E2 B4BF 4530 D39B 2D51

Re: [PATCH v2] Suppress unavoidable memrchr() warning

[Michal S. <msu...@su...>]

On Thu, Apr 16, 2026 at 11:45:47AM +0300, Evgeniy Berdnikov wrote:
>   Hello.
>   
> On Thu, Apr 16, 2026 at 06:12:24PM +1000, Seth McDonald via isync-devel wrote:
> > The derivation of B from A thus requires casting away the const, which
> > produces a compilation warning in environments using our memrchr()
> > implementation.  For all intents and purposes this warning is
> > unavoidable, so locally supress it for the relevant cast.
> ...
> >  void *
> >  memrchr( const void *s, int c, size_t n )
> >  {
> > +DIAG_PUSH
> > +DIAG_DISABLE("-Wcast-qual")
> >  	u_char *b = (u_char *)s, *e = b + n;
> > +DIAG_POP
> >  
> >  	while (--e >= b)
> >  		if (*e == c)
> 
>  BTW, there is a compiler-independent way to suppress such warning:
> 
>         union { u_char *b; const void *cvoid_b; } ub;
> 
>         ub.cvoid_b = s;
>         u_char *e = ub.b + n;
>         while (--e >= ub.b)
>                 if (*e == c)
>         ...
> 
>  It assumes that pointers to all bare types are represented identically,
>  AFAIK, this is true for all existent C implementations.

Also it breaks the aliasing rules that C fabricates regardless of the
actual use of the data.

As in b and cvoid_b being different types makes them independent
variables regardless of them accidentally occuppying the same memory,
and C is free to flag ub.b as unassigned, optimize out the assignment to
ub.cvoid_b that is never used, etc.

Thanks

Michal

Re: [PATCH v2] Suppress unavoidable memrchr() warning

[Oswald B. <os...@us...>]

On Thu, Apr 16, 2026 at 06:12:24PM +1000, Seth McDonald wrote:
>(Why not make B const?  Idfk ask the glibc devs.)
>
because that's most convenient on the user side (_somebody_ has to do 
that cast if the object is to be modified), and consistent with the 
"normal" memchr(), strchr(), and other similar functions.

c23 aims to resolve this issue using a c11 feature.
there is a pertinent thread on the mutt-dev list right now:
https://marc.info/?l=mutt-dev&m=177631784909117&w=2 and 
https://marc.info/?l=mutt-dev&m=177632421112239&w=2 .

regarding the commit message itself, it's way too wordy for my taste 
now, but still misses the key point: how does the implementation compare 
to what glibc itself actually does?

Re: [PATCH v2] Suppress unavoidable memrchr() warning

[Evgeniy B. <bi...@pr...>]

  Hello.
  
On Thu, Apr 16, 2026 at 06:12:24PM +1000, Seth McDonald via isync-devel wrote:
> The derivation of B from A thus requires casting away the const, which
> produces a compilation warning in environments using our memrchr()
> implementation.  For all intents and purposes this warning is
> unavoidable, so locally supress it for the relevant cast.
...
>  void *
>  memrchr( const void *s, int c, size_t n )
>  {
> +DIAG_PUSH
> +DIAG_DISABLE("-Wcast-qual")
>  	u_char *b = (u_char *)s, *e = b + n;
> +DIAG_POP
>  
>  	while (--e >= b)
>  		if (*e == c)

 BTW, there is a compiler-independent way to suppress such warning:

        union { u_char *b; const void *cvoid_b; } ub;

        ub.cvoid_b = s;
        u_char *e = ub.b + n;
        while (--e >= ub.b)
                if (*e == c)
        ...

 It assumes that pointers to all bare types are represented identically,
 AFAIK, this is true for all existent C implementations.
-- 
 Eugene Berdnikov

[PATCH v2] Suppress unavoidable memrchr() warning

[Seth M. <de...@se...>]

In case the standard library does not provide the memrchr() function (a
GNU extension), we define our own implementation of the function.  To
maintain compatibility with glibc, the function must receive a const
pointer A and return a non-const pointer B derived from A.  (Why not
make B const?  Idfk ask the glibc devs.)

The derivation of B from A thus requires casting away the const, which
produces a compilation warning in environments using our memrchr()
implementation.  For all intents and purposes this warning is
unavoidable, so locally supress it for the relevant cast.
---
 src/util.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/util.c b/src/util.c
index c110188d4fac..fc7f49899634 100644
--- a/src/util.c
+++ b/src/util.c
@@ -454,7 +454,10 @@ vasprintf( char **strp, const char *fmt, va_list ap )
 void *
 memrchr( const void *s, int c, size_t n )
 {
+DIAG_PUSH
+DIAG_DISABLE("-Wcast-qual")
 	u_char *b = (u_char *)s, *e = b + n;
+DIAG_POP
 
 	while (--e >= b)
 		if (*e == c)

base-commit: 9aaac66286910f547ec3068d3fd72afb4fe716bf
-- 
2.50.1 (Apple Git-155)

Re: [PATCH 0/2] Support coloured output

[Oswald B. <os...@us...>]

On Tue, Apr 14, 2026 at 04:34:49PM +1000, Seth McDonald wrote:
>One of the reasons for my per-attribute disabling
>was to allow some amount of these nestings.  Where, for example, msg1 is
>bold and msg2 is red, and msg2 contains msg1.  By working per-attribute,
>the end of msg1 would not remove the red colouring of msg2.
>
i see how this could be useful in principle, but i suspect that this 
isn't a realistic use case in isync.

given that the usage of the actual coloring api is rather 
"concentrated", it wouldn't cause much churn to switch to a fully 
fledged api at a later point. so i would start with the minimal 
implementation.

>> however, another feature would actually make a lot of sense, to 
>> enable
>> elegant (and slightly more efficient) wrapping of messages: recursive format
>> strings. think
>[...]
>> i have wanted this so many times already that i wonder why the standard
>> doesn't have it in some form.
>
>If I had to guess, probably because it sounds like a security nightmare
>for implementors and users. [...]
>
that's a valid concern, but i think it's unfounded. you just have to 
apply the same rules to the nested format strings as to the top-level 
ones, i.e., pass only trusted strings (generally, constant string 
literals). security constraints are always applied top-down; never 
leaving a gap is just something you have to keep in mind.

i suspect it might be actually an implementation problem. i think we'd 
need something like pointers into va_list or the ability to push the 
current state onto a stack, and i'm not convinced that either is 
actually a thing, at least portably. Somebody (TM) would have to give it 
a shot ...

>Though for specific queries about the code, is this list appropriate 
>for asking?.
>
this list is just fine.
you can also try to catch me on matrix, @ossi:kde.org, or ossi on 
libera.chat irc (which i use via the matrix bridge).

>Also, it's probably becoming apparent that I'm generally not able to
>reply quickly due to the difference in time zones.
>
yeah, aussie time is just about the worst for me. you can catch me in 
the late evening if you get up early, but then you'd actually have to 
use im, as at that time of day i read email only sporadically.

>Hopefully that isn't too much of a problem :)
>
for me it's fine, i'm in no hurry ...

Re: [PATCH] Suppress unavoidable memrchr() warning

[Oswald B. <os...@us...>]

On Tue, Apr 14, 2026 at 08:02:36PM +1000, Seth McDonald wrote:
>To my knowledge, memrchr() is a nonstandard GNU extension,
>
yes, the linux man page says so.
but then the obvious followup is "do whatever glibc does", and mention 
that in the commit message.

Re: [PATCH] Suppress unavoidable memrchr() warning

[Seth M. <de...@se...>]

Hi Michal,

On Tue, 14 Apr 2026 at 11:28:41 +0200, Michal Suchánek wrote:
> On Tue, Apr 14, 2026 at 06:51:36PM +1000, Seth McDonald via isync-devel wrote:
> > Our memrchr() implementation must cast away const from the 's'
> > parameter, which causes a compiler warning.  Locally suppress this
> > warning for the relevant cast.
> 
> it actually does not have to.
> 
> It's quite possible to make the return value also const.
> 
> It's not how the standard defines the function but not everything in the
> standard makes sense.

To my knowledge, memrchr() is a nonstandard GNU extension, so it isn't
"as bad" to change the function signature.  However, we only have a
definition of the function for the situation in which the provided
standard library does not include it.  So if we change the signature for
our implementation, then the function's signature will be different in
different compilation environments:

- If memrchr() is present in the standard library, the signature will
  have a non-const return value.
- If memrchr() is not present, the signature will instead have a const
  return value.

While I agree a const return value would be better, having different
signatures in different environments would be worse.  A single invariant
codebase makes debugging and portability much easier.  Which in this
case may manifest as ensuring compilation does not succeed in only some
environments but fail in others.

Take care,
	Seth McDonald.

-- 
E9D1 26A5 F0D4 9DF7 792B  C2E2 B4BF 4530 D39B 2D51

Re: [PATCH] Suppress unavoidable memrchr() warning

[Michal S. <msu...@su...>]

Hello,

On Tue, Apr 14, 2026 at 06:51:36PM +1000, Seth McDonald via isync-devel wrote:
> Our memrchr() implementation must cast away const from the 's'
> parameter, which causes a compiler warning.  Locally suppress this
> warning for the relevant cast.

it actually does not have to.

It's quite possible to make the return value also const.

It's not how the standard defines the function but not everything in the
standard makes sense.

If you care about warnings and the code problems they point out using
const return value sounds like a better option.

Thanks

Michal

[PATCH] Suppress unavoidable memrchr() warning

[Seth M. <de...@se...>]

Our memrchr() implementation must cast away const from the 's'
parameter, which causes a compiler warning.  Locally suppress this
warning for the relevant cast.
---
 src/util.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/util.c b/src/util.c
index c110188d4fac..fc7f49899634 100644
--- a/src/util.c
+++ b/src/util.c
@@ -454,7 +454,10 @@ vasprintf( char **strp, const char *fmt, va_list ap )
 void *
 memrchr( const void *s, int c, size_t n )
 {
+DIAG_PUSH
+DIAG_DISABLE("-Wcast-qual")
 	u_char *b = (u_char *)s, *e = b + n;
+DIAG_POP
 
 	while (--e >= b)
 		if (*e == c)

base-commit: 9aaac66286910f547ec3068d3fd72afb4fe716bf
-- 
2.50.1 (Apple Git-155)

Re: [PATCH 0/2] Support coloured output

[Seth M. <de...@se...>]

On Mon, 13 Apr 2026 at 12:33:42 +0200, Oswald Buddenhagen via isync-devel wrote:
> i had that thought as well, but i'm actually thinking yet more radically
> minimalistic:
> 
>  ...
>  #define COLOR_WARNING "\x1b[33m"
>  #define COLOR_ERROR "\x1b[31m"
>  #define RESET_COLOR "\x1b[0m"

The main disadvantage to this approach is if you want to check or change
these values (e.g. change warnings from yellow to bright yellow), you'd
have to look up the SGR escape codes, find the desired values, add them,
and test to ensure they were correct.  As opposed to just changing a
flag on a function.

Of course, I assume it's unlikely the colour scheme would change much in
the future, so this isn't by itself an argument against this approach;
it's just something I think important to note.

Btw, if using this approach, you can shorten consecutive escape
sequences by separating the parameter values with semicolons.  For
example, to specify "bold and red", rather than using "\x1b[1m\x1b[31m",
you can use "\x1b[1;31m".

> > 	xprintf( "%?hello!%?\n", SGR_BOLD, SGR_NO_BOLD );
> > 
> yes, something like that, with automatic handling of disabled coloring.

If we're going to be auto-disabling display attributes with (presumably)
the "\x1b[0m" escape sequence, then that does limit the use of nested
coloured messages.  One of the reasons for my per-attribute disabling
was to allow some amount of these nestings.  Where, for example, msg1 is
bold and msg2 is red, and msg2 contains msg1.  By working per-attribute,
the end of msg1 would not remove the red colouring of msg2.

Btw, you can actually shorten the escape sequence "\x1b[0m" to just
"\x1b[m".  If you don't give a parameter, it just assumes zero.

> however, another feature would actually make a lot of sense, to enable
> elegant (and slightly more efficient) wrapping of messages: recursive format
> strings. think
[...]
> i have wanted this so many times already that i wonder why the standard
> doesn't have it in some form.

If I had to guess, probably because it sounds like a security nightmare
for implementors and users.  To be clear, I'm not at all a security
expert or anything.  But it feels like something that could open a
plethora of possible attacks that would need to be accounted for.
Things like injecting %s and %n sequences, and misusing or miscounting
the nested variadic arguments come to mind, for instance.

I think recursive format strings should definitely be something we
should stay away from for this reason.  At least not until there's some
_strong_ confirmation that they are indeed safe to use.  I personally
would not be too comfortable using them without such confirmation.

> > Sure, I was just trying to avoid the situation where errors or warnings
> > may occur before and after parsing the --color option, which would cause
> > the former output to not be coloured, while the latter output would be
> > (or vice versa).
> > 
> the whole point of the coloring is to make it easier to spot significant
> information in large amounts of output. so let's consider the cases:
> if there is an error, the run ends right at startup. there isn't any other
> output.

That's a good point; I hadn't considered that.  So the only
inconsistency would be between separate program executions with
different command-line options, which doesn't nearly matter as much (if
at all, arguably).

> config parsing starts only now. that might become a minor problem if
> somebody really needs a persistent version of the option, but for the time
> being i have no intention of adding that.

For persistent coloured output, if the default auto-detection doesn't
detect support for the tty, you can just define a shell alias:

	alias mbsync="mbsync --color"

That's what I do for grep, ls, etc.

> looking at patches can be a useful way to familiarize oneself with a
> project, because they offer a narrow, directed entry point.

Nice idea.  I'll have a look at the patches associated with relevant
code (identified via git-blame or git-log).  There's also the mailing
list archives I can check out.

Though for specific queries about the code, is this list appropriate for
asking?  Things like "what's the purpose of this variable?" or "why use
this rather than that?".  Asked after RTFM, of course.

Also, it's probably becoming apparent that I'm generally not able to
reply quickly due to the difference in time zones.  Hopefully that isn't
too much of a problem :)

Take care,
	Seth McDonald.

-- 
E9D1 26A5 F0D4 9DF7 792B  C2E2 B4BF 4530 D39B 2D51

Re: [PATCH 1/2] Add API to support escape sequences

[Evgeniy B. <bi...@pr...>]

On Mon, Apr 13, 2026 at 12:44:17PM +0200, Oswald Buddenhagen via isync-devel wrote:
> On Mon, Apr 13, 2026 at 01:24:54PM +0300, Evgeniy Berdnikov wrote:
> > Yes, I think good approach is to do things right,
> > if implementation is not very complex.
> > 
> indeed, and that's where things get utterly out of hand.
> 
> if we wanted to go beyond the proposed absolutely trivial heuristics, we
> should do it properly and use termcap/terminfo. i would like to avoid that
> until ncurses proper is on the table

 I agree, we should avoid heavy code with terminfo (termcap is deprecated).
 
 After some googling I've got an impression that almost all modern terminal
 emulators do supports vt100-like escape sequences. So we do not need real
 selectivity for terminal type today. I withdraw my proposal.

 PS. My short research results in this list of terminal name prefixes,
 indicating vt100-escapes support: "xterm", "vt100", "linux", "rxvt",
 "putty", "konsole", and lot of names starting with "vt"+digits.
-- 
 Eugene Berdnikov

Re: [PATCH 1/2] Add API to support escape sequences

[Oswald B. <osw...@gm...>]

On Mon, Apr 13, 2026 at 01:24:54PM +0300, Evgeniy Berdnikov wrote:
> Yes, I think good approach is to do things right,

> if implementation is not very complex.
>
indeed, and that's where things get utterly out of hand.

if we wanted to go beyond the proposed absolutely trivial heuristics, we 
should do it properly and use termcap/terminfo. i would like to avoid 
that until ncurses proper is on the table (which would be useful for 
full-screen visualization of massive parallelism, but why would one want 
that level of complexity in a tool that is mostly used in background 
jobs?).

Re: [PATCH 0/2] Support coloured output

[Oswald B. <os...@us...>]

On Mon, Apr 13, 2026 at 04:54:41PM +1000, Seth McDonald wrote:
>It is true that sgr_attr_t isn't actually needed; the
>ds_to_sgr function pointer could just return the escape sequence string
>directly (e.g. returning "\x1b[31m" rather than 31).
>
i had that thought as well, but i'm actually thinking yet more radically 
minimalistic:

  ...
  #define COLOR_WARNING "\x1b[33m"
  #define COLOR_ERROR "\x1b[31m"
  #define RESET_COLOR "\x1b[0m"

that's the big advantage of limiting ourselves to basic SGR sequences.  
no abstractions needed.

>	xprintf( "%?hello!%?\n", SGR_BOLD, SGR_NO_BOLD );
>
yes, something like that, with automatic handling of disabled coloring.
but this is probably over-engineered; it doesn't really add much value 
as-is.

however, another feature would actually make a lot of sense, to enable 
elegant (and slightly more efficient) wrapping of messages: recursive 
format strings. think

   void err(const char *fmt, ...)
   {
	va_list va;
	char date[32];

	va_start( va, fmt );
	fmt_date(date);
	xprintf( "%s " COLOR_ERROR "error: %&" RESET_COLOR "\n", date, fmt, va );
	va_end( va );
   }

i have wanted this so many times already that i wonder why the standard 
doesn't have it in some form.

>Though on the other hand, this would increase usage of a custom printf()
>implementation over the standard library's.  Which, since the library's
>is likely much more optimised, may incur a slight performance hit.
>
the only thing one can do really badly in that context is to pass every 
char separately to the output layer. or do lots of dynamic allocations.  
we don't do either.

but realistically, performance differences in the printf implementation 
are entirely negligible in this application.

>Sure, I was just trying to avoid the situation where errors or warnings
>may occur before and after parsing the --color option, which would cause
>the former output to not be coloured, while the latter output would be
>(or vice versa).
>
the whole point of the coloring is to make it easier to spot significant 
information in large amounts of output. so let's consider the cases:
if there is an error, the run ends right at startup. there isn't any 
other output.
warnings are printed only after the parsing completed, at which point 
coloring can be already enabled.
config parsing starts only now. that might become a minor problem if 
somebody really needs a persistent version of the option, but for the 
time being i have no intention of adding that.

>> i recommend that you take a look at all wip/ branches, [...]
>
>I typically just look the master/main branch as it can sometimes be
>difficult to determine which branches are still in development and which
>are done or abandoned (in general, not this repo specifically).  But I
>can take a look at those wip branches if they're still open for
>development.
>
everything in the repo should be still relevant somehow.
most branches are "indefinitely deferred" for one reason or another.  
the commit message of the initial (and usually only) commit should 
explain why, though this isn't always the case.
if you see anything that appears truly abandoned (or obsolete), let me 
know.

>Though I am still trying to wrap my head around much of the code in the
>master branch, so it may be a bit before I rigorously examine some other
>branches.
>
looking at patches can be a useful way to familiarize oneself with a 
project, because they offer a narrow, directed entry point. it's 
basically a bottom-up approach. of course, this isn't useful if the 
topics handled there have little/no overlap with what actually interests 
you.

Re: [PATCH 1/2] Add API to support escape sequences

[Evgeniy B. <bi...@pr...>]

  Hello.
  
On Mon, Apr 13, 2026 at 03:02:08PM +1000, Seth McDonald wrote:
> On Sun, 12 Apr 2026 at 19:56:41 +0300, Evgeniy Berdnikov wrote:
> >  Absence of terminal is not an error, this is normal situation, such as run
> >  in batch job or systemd service.
> 
> Indeed.  That's why it's checking if errno is _not_ ENOTTY.

 Sorry, you're right, I misread the code.

> >  You have better to check explicitly at least for "xterm", "linux" and "vt100"
> >  (with account for suffixes after "-" character, as term(7) and terminfo(5)
> >  stated), and do not use vt100-like escapes for unknown/undefined terminals.
> 
> So you're saying we should default to not using escape sequences unless
> the terminal is known to support them (as opposed to defaulting to using
> them unless the terminal is known to not support them)?  I'd imagine
> we'd have to cover a lot of cases to be portable, which is possible, but
> IMO not really the best use of time.

 Yes, I think good approach is to do things right, if implementation is
 not very complex. For this case code may be like this:

 int detect_display_attrs(..) {
     ...
     char *term_names[] = { "vt100", "xterm", "linux", NULL };
     int prefix_len;
     char *minus_ptr, *n;
     ...
     minus_ptr = strchrnul(term, '-');
     prefix_len = (int)(minus_ptr - term);
    
     for (n = term_names[0]; n; n++) {
             if (!strncmp(term, n, prefix_len))
         	 	return 1;
     }
     return 0;
 }
-- 
 Eugene Berdnikov

Re: [PATCH 0/2] Support coloured output

[Seth M. <de...@se...>]

Hi Oswald,

On Sun, 12 Apr 2026 at 14:16:04 +0200, Oswald Buddenhagen via isync-devel wrote:
> On Sun, Apr 12, 2026 at 04:14:21PM +1000, Seth McDonald wrote:
> > This patchset aims to tick off the following from the TODO file.
> > 
> > | possibly use ^[[1m to highlight error messages.
> > 
> cool.
> 
> my first reaction is "huh, this is completely over-engineered".

Yeah I can see that lol.  My goal was to add an API (well, one function)
that's simple to use (hence the bitfield) and requires minimal change to
the existing codebase to use (hence the message "wrapping").

> i'd just go
> with a hard-coded SGR sequence #define for each message class and a common
> reset sequence rather than an elaborate attribute translation layer.

You're referring to the transition between display_attr_enum and
sgr_attr_t?  It is true that sgr_attr_t isn't actually needed; the
ds_to_sgr function pointer could just return the escape sequence string
directly (e.g. returning "\x1b[31m" rather than 31).  I think I
initially included sgr_attr_t to make it easier to represent escape
sequences (it's easier to store integers than strings).  But in
hindsight that benefit doesn't really manifest given I immediately
convert any sgr_attr_t values to their corresponding string
representation.  So I think I agree that directly converting from
display_attr_enum values to escape strings (with #defines) is better.

> the wrapping of the "base calls" is a good (and somewhat obvious) idea.  but
> for the implementation, we may end up with something rather different:
> - we may be able to inject the sequences more directly by virtue of   having
> our own printf() implementation. that's just an idea, not an   instruction.

xprintf()?  I haven't yet looked too much into that, but it seems to be
similar to printf() but with extra format sequences and a trailing
callback.  If this were the way to go, I would imagine a specific format
sequence to insert an escape sequence could work.  Something like:

	xprintf( "%?hello!%?\n", SGR_BOLD, SGR_NO_BOLD );

Though on the other hand, this would increase usage of a custom printf()
implementation over the standard library's.  Which, since the library's
is likely much more optimised, may incur a slight performance hit.
That's just a guess though; I haven't done any benchmarking.

> there is no need for the init_colors() hack. coloring the command line
> parser output adds no value, so it's fine to just postpone coloring
> activation.

Sure, I was just trying to avoid the situation where errors or warnings
may occur before and after parsing the --color option, which would cause
the former output to not be coloured, while the latter output would be
(or vice versa).  But if that's not a concern then the logic can
certainly be simplified.

> please use american english throughout. it's not that i like it, but that's
> what the "C" locale really is, and consistency is king.

Of course, forgot to include the US-defaultism ;)

> as you're certainly noticing at this point, i'm very particular about how
> things should be implemented. to avoid wasting time and mental energy, it's
> best to discuss designs in advance. i think the "show me the code" approach
> many projects go with is extremely disrespectful towards contributors for
> anything but the most trivial patches.

Good to know; I'm glad communication is welcomed :).  And I'll be happy
to propose any ideas prior to fully implementing them (other than simple
typos/fixes, of course).

> i recommend that you take a look at all wip/ branches, to avoid redundancy
> and divergent developments (beyond what happened since the branches
> sprouted, anyway - some a rather old). these aren't a priority by any means,
> but it may make sense to drive some of them to completion before doing other
> stuff, not last because many of them are (based on) external contributions,
> so obviously somebody cared enough to give it a shot.

I typically just look the master/main branch as it can sometimes be
difficult to determine which branches are still in development and which
are done or abandoned (in general, not this repo specifically).  But I
can take a look at those wip branches if they're still open for
development.

Though I am still trying to wrap my head around much of the code in the
master branch, so it may be a bit before I rigorously examine some other
branches.

Take care,
	Seth McDonald.

-- 
E9D1 26A5 F0D4 9DF7 792B  C2E2 B4BF 4530 D39B 2D51

Re: [PATCH 1/2] Add API to support escape sequences

[Seth M. <de...@se...>]

Hi Evgeniy,

On Sun, 12 Apr 2026 at 19:56:41 +0300, Evgeniy Berdnikov wrote:
> On Sun, Apr 12, 2026 at 04:14:22PM +1000, Seth McDonald via isync-devel wrote:
> > +	errno = 0;
> > +	if (!isatty( fd )) {
> > +		if (errno != ENOTTY)
> > +			sys_error( "Cannot examine file descriptor %d", fd );
> 
>  Absence of terminal is not an error, this is normal situation, such as run
>  in batch job or systemd service.

Indeed.  That's why it's checking if errno is _not_ ENOTTY.  See the
POSIX.1-2024 entry for isatty()[0]:

| The isatty() function may fail if:
[...]
| [ENOTTY]
| The file associated with the fildes argument is not a terminal.

If fd is not a terminal, errno will be set to ENOTTY, meaning the error
message will not appear.  But if errno is not set to ENOTTY, then the
isatty() call failed for a reason other than "not a terminal" (probably
an invalid file descriptor).  This is an error that should be logged,
because it indicates something is wrong with the program state.

> > +	const char *term = getenv( "TERM" );
> > +	if (term && !strcmp( term, "dumb" ))
> > +		return 0;
> 
>  You are assuming all terminals different from "dumb" do support vt100-like
>  escape sequences. This is almost true, because other terminals are
>  historical artefacts. Moreover, in case of undefined TERM this code produces
>  escape sequences. I think it should not be.

I actually just attempted to mimic the behaviour of git-status here.  I
ran `TERM=... git status` a few times, with TERM set to different
values, and found that - of the values I tested - only TERM=dumb removed
the coloured output.

I figured it would be better to defer the wanted behaviour to those of
common existing programs, as this would integrate nicely in environments
that users may have already curated for such behaviour.  Though I am
open to adding custom behaviour if that's what's wanted.

>  You have better to check explicitly at least for "xterm", "linux" and "vt100"
>  (with account for suffixes after "-" character, as term(7) and terminfo(5)
>  stated), and do not use vt100-like escapes for unknown/undefined terminals.

So you're saying we should default to not using escape sequences unless
the terminal is known to support them (as opposed to defaulting to using
them unless the terminal is known to not support them)?  I'd imagine
we'd have to cover a lot of cases to be portable, which is possible, but
IMO not really the best use of time.

Take care,
	Seth McDonald.

[0] <https://pubs.opengroup.org/onlinepubs/9799919799/functions/isatty.html>

-- 
E9D1 26A5 F0D4 9DF7 792B  C2E2 B4BF 4530 D39B 2D51