Hi Oswald,
I appreciate the timely reply!
On Wed, 01 Apr 2026 at 12:38:11 +0200, Oswald Buddenhagen wrote:
> On Wed, Apr 01, 2026 at 02:44:24PM +1000, Seth McDonald wrote:
> > [PATCH 1/4] Remove excessive if nesting
> >
> > Such control flow is not the most readable, particularly for longer
> > functions, due to the added indentation.
> >
> that's true, but irrelevant for short functions. this was violated a bit in
> ensure_password due to the later addition of the macos code, but you're
> rectifying this in the next patch. this really makes this patch
> counter-productive churn imo. please drop it.
Sure, I'll send in a v2 without it.
> fwiw, isync's indentation is tab width independent, but it's visually
> optimized for four spaces.
Good to know. I initially had the impression it was intended to
represent eight spaces, which influenced some of the patches' formatting
(e.g. the 80-column limit I tend to use). I'll adjust the formatting
accordingly.
> unlike many other projects, i write commit messages such that they actually
> match the idea of describing a transition, so there is no ambiguous
> "current" state that relies on convention.
That's certainly a first for me, but I can respect it.
> > The SecItem API requires the creation of multiple objects to specify the
> > search query, each of which may fail for whatever reason (most likely
> > OOM).
>
> > For now, use assertions to ensure no such object creation fails.
> >
> yeah, that's just ugly.
> just swap the two patches.
Just to clarify, you're referring to patches 3 and 4?
Also, I will note that after patch 4, there is still an assert() for
'query'. I left it in because I didn't think adding a no-fail wrapper
for CFDictionaryCreate() was as useful as the other wrappers. Mainly
because it can reasonably fail for reasons other than OOM, such as
invalid arguments. Though if that isn't a concern, then I can replace
that assert() as well.
> > search_macos_keychain( const char *host, const char *user )
> > {
> > - void *password_data;
> > + CFDictionaryRef query;
> >
> please localize the declarations more consistently.
I'm unsure what you mean here. I attempted to group related
declarations next to one another, and otherwise order them by which gets
assigned to/used first.
> > + /* Same index = key-value pair */
> >
> prefer c99 style for inline comments.
> (traditional style is ok for function docs.)
Sure thing.
Take care,
Seth McDonald.
--
E9D1 26A5 F0D4 9DF7 792B C2E2 B4BF 4530 D39B 2D51
_______________________________________________
isync-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/isync-devel
