On Fri, Jul 24, 2026 at 02:37:41PM +1000, Seth McDonald wrote:
On Wed, 22 Jul 2026 at 14:57:28 +0200, Oswald Buddenhagen wrote:
On Sat, Jul 18, 2026 at 03:29:40PM +1000, Seth McDonald via isync-devel wrote:
> The interface's implementation adds minor changes in behaviour to
> simplify the authentication logic. This includes always attempting to
> fall back to LOGIN when SASL fails and unifying similar error messages.
>
semantically, that seems just wrong. you need to work a lot harder to
convince me that this is a wanted change.
The primary motivation is that the current "flow" of the SASL logic is
incompatible with the GNU SASL library.
upon a cursory look at the gsasl documentation, i don't see how that
would be fundamentally true.
in fact, i have the impression that you didn't study it particularly
thoroughly, given that you thought it necessary to hard-code the list of
available mechanisms.
a different angle is that it isn't necessary to utilize the library's
full potential. it's just fine to duplicate a bit of the library's
functionality if it makes the implementations more uniform. e.g., i
don't see any practical benefit in actually using gsasl's LOGIN support.
Regarding the falling back to LOGIN, it simplifies the logic from
unnecessarily complex "if it fails in this specific way, goto here; but
if it fails like this, goto there" to "if it fails, try LOGIN instead".
that description of the status quo doesn't fill me with confidence that
you actually understand it.
> +static const char *
> +query_sasl_error( sasl_conn_t *ctx, int rc )
>
i don't want that obfuscation layer without good reason.
I added it for two reasons:
(0) It's simpler to have a single function to call to retrieve the
appropriate error message, rather than manually switching
between sasl_errdetail() and sasl_errstring() all the time.
it's two functions for a good reason, and i think that blurring the two
contexts is actively counter-productive.
(1) To encompass the following fallback.
> + const char *err = ctx
> + ? sasl_errdetail( ctx )
> + : sasl_errstring( rc, NULL, NULL );
> + return err ? err : "Unknown error";
>
what is that fallback for?
From the documentation for sasl_errdetail() (see [0]):
| Returns the string describing the error that occurred, or NULL if no
| error has occurred, or there was an error retrieving it.
calling the function when no error occurred would be an internal error
in isync.
and error while retrieving an error string would be an internal error in
libsasl or the calling code.
in either case i want that to be glaringly obvious, not papered over.
> +static int
> +process_sasl_step(
> + } else {
> + const char *err = query_sasl_error( session, rc );
>
i don't like these pointless temporaries.
I tend to use them for a few reasons:
(0) They make debugging line-by-line with GDB easier by keeping calls on
their own lines, which allows for easy stepping into/over select
calls. This is especially true for function calls inside the
conditions of if-statements.
this is true, but completely irrelevant for the "print stringified error
code" pattern.
(1) They make git diff/blame cleaner by separating operations with
newlines, which allows diff/blame to better identify and show
the exact operations that were changed.
that's equally achievable by wrapping the "resolver" call to the next
line. but in this context it's also rather irrelevant.
(2) They tell you what exactly you're retrieving from the function,
which may not always be so obvious. For example, if a call
returns a status, does zero indicate success or failure? A line
like `if (!foo())` doesn't tell you much, but a line like
`int bad = !foo();` tells you immediately it means failure.
also completely irrelevant in this context.
so same principle as everywhere: don't be verbose unless it adds actual
value.
and both of the above deviations from the established practice in isync
have absolutely no business in a commit that purports to do something
different.
> + intersect_mechs(
> + common_mechs + 1,
> + size - 1,
> + any_mech ? server_mechs : client_mechs,
> + server_mechs );
>
that call formatting is rarely used in isync, and looks weird even with the
smaller tabs. don't use it without strong reason (long argument list
starting beyond column ~60, or arguments with long comments).
For context, my personal rule is: if it's within 80 columns, format as
one line; otherwise, split the arguments as a separate line if they're
within 80 columns; otherwise, have each argument on a separate line.
well, isync has no hard line length limit, as that just makes no sense
in conjunction with a variable tab width.
in practice i'm aiming for below 120 cols with 4-space tabs, but there
isn't really a rule when to wrap. logical argument grouping and
surrounding line lengths are key factors.
(I like the 80-column limit as it makes side-by-side diffs a lot lot
easier to view.)
i consider side-by-side diffs irrelevant. i sometimes switch to that view
when the code has been completely rewritten, i.e., there isn't actually
a meaningful local diff to begin with. in this situation, having to
side-scroll isn't all that bad (though still annoying, of course).
but i forgot to mention something before: judging by the sub-optimality
of some of the hunks, you don't seem to have switched your git diff
algorithm to histogram yet?
So using this case as an example, would you prefer a more compact
formatting like so:
intersect_mechs(
common_mechs + 1, size - 1,
any_mech ? server_mechs : client_mechs, server_mechs );
yes, i would.
but that doesn't change the fact that deviating from the predominant
style of the code base needs to be well-justified in the first place.
> +typedef const char *(*imap_auth_cred_t)( void * );
> +
the consequences of having these are obviously ugly.
just factor out imap_auth_conf_t as i hinted at previously.
Sure, but just FYI without it the logic will no longer be
self-contained. It'll depend on the implementation of the IMAP driver,
why would it?
and thus won't be able to be understood or maintained purely as a
standalone unit (which was partly my goal).
i don't see that as a goal per se.
> +/*
> + * Formats a list of SASL mechanisms supported by both client and server
side.
> + * The list is space-delimited, including starting and ending with
whitespace.
> + * Mechanisms are parsed in a case-insensitive manner.
> + */
> +void imap_auth_filter_mechs(
>
this seems like an implementation detail that should not leak through the
abstraction.
Not sure I entirely agree.
The case-insensitive detail [...]
The space-delimited detail [...]
you didn't pay attention to the reply placement ...
i question the function being part of the iface in the first place.
> +char *
> +find_string_list( const string_list_t *list, const char *str )
> +
> +char *
> +find_string_list_case( const string_list_t *list, const char *str )
the return values make no sense. these should be bools, and the functions
called contains_....
I was aiming for a function analogous to strstr(). It's purpose is to
find and, if found, return a string within a string list. And like
strstr(), it can be used secondarily to only check if the string list
contains the string by checking if the return value is NULL.
don't try to make analogous interfaces when there is a cost, but no
obvious advantage for existing use cases.
related to other comments, i find it debatable that factoring these
out
actually makes sense. but if it does, it would have to happen in a separate
patch.
So you're okay with patches that add general functions but don't include
their usage until later patches?
that's not at all what i said.
if you want to refactor that aspect, then do it in a separate commit.
only then the effect of the _change_ can be judged without it being
completely drowned out by the surrounding bigger changes.
your prior macos keychain patches should be integrated into this
series to
reduce overall churn. for that purpose, it's ok to either
- during refactoring, leave code in its old place when it will be replaced
in subsequent patches, or
- to add code in a new file that will be logically created only in the
subsequent refactoring (i think this is the case applicable here).
I'm afraid I don't see how integrating the keychain patches into this
patchset will reduce churn. The imap_auth_* interface doesn't touch nor
take responsibility for either the keychain code or the
password-querying logic as a whole. It calls (directly or indirectly)
ensure_password(), but doesn't care how that function works.
that argument is just wrong in light of the imap_auth_conf_t discussion
above.
For (more) context about me, I like to explicitly document functions'
effects that are relied upon in the codebase. This is because when
analysing code, I find it very useful to know exactly what a function is
_supposed_ to do, and to contrast that with what it _actually_ does. I
don't assume everyone can infer all my intentions (including myself),
however obvious I think it to be.
sure, but there is a cost to maintaining that, and even reading it
(because of needing to verify the ongoing validity). it just doesn't pay
off at the scale and process requirements we are dealing with here.
_______________________________________________
isync-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/isync-devel
