On Tue, Apr 22, 2025 at 3:02 AM Daniel Gustafsson <dan...@yesql.se> wrote:
> + if oauth_flow_supported
> + cdata.set('USE_LIBCURL', 1)
> + elif libcurlopt.enabled()
> + error('client OAuth is not supported on this platform')
> + endif
> We already know that libcurlopt.enabled() is true here so maybe just doing
> if-else-endif would make it more readable and save readers thinking it might
> have changed?Features are tri-state, so libcurlopt.disabled() and libcurlopt.enabled() can both be false. :( My intent is to fall through nicely in the case where -Dlibcurl=auto. (Our minimum version of Meson is too old to switch to syntax that makes this more readable, like .allowed(), .require(), .disable_if(), etc...) > Also, "client OAuth" reads a bit strange, how about "client-side > OAuth" or "OAuth flow module"? > ... > I think we should take this opportunity to turn this into a > appendPQExpBuffer() > with a format string instead of two calls. > ... > Now that the actual variable, errbuf->len, is short and very descriptive I > wonder if we shouldn't just use this as it makes the code even clearer IMO. All three done in v9, attached. Thanks! --Jacob
1: 5f87f11b18e = 1: 5f87f11b18e Add minor-version counterpart to
(PG_)MAJORVERSION
2: 4c9cc7f69af ! 2: 9e37fd7c217 oauth: Move the builtin flow into a separate
module
@@ configure.ac: if test "$PORTNAME" = "win32" ; then
+if test "$with_libcurl" = yes ; then
+ # Error out early if this platform can't support libpq-oauth.
+ if test "$ac_cv_header_sys_event_h" != yes -a
"$ac_cv_header_sys_epoll_h" != yes; then
-+ AC_MSG_ERROR([client OAuth is not supported on this platform])
++ AC_MSG_ERROR([client-side OAuth is not supported on this platform])
+ fi
+fi
+
@@ meson.build: if not libcurlopt.disabled()
+ if oauth_flow_supported
+ cdata.set('USE_LIBCURL', 1)
+ elif libcurlopt.enabled()
-+ error('client OAuth is not supported on this platform')
++ error('client-side OAuth is not supported on this platform')
+ endif
+
else
@@ src/interfaces/libpq-oauth/oauth-curl.c:
pg_fe_run_oauth_flow_impl(PGconn *conn)
* also the documentation for struct async_ctx.
*/
if (actx->errctx)
- {
+- {
- appendPQExpBufferStr(&conn->errorMessage,
-
libpq_gettext(actx->errctx));
- appendPQExpBufferStr(&conn->errorMessage, ": ");
-+ appendPQExpBufferStr(errbuf, libpq_gettext(actx->errctx));
-+ appendPQExpBufferStr(errbuf, ": ");
- }
+- }
++ appendPQExpBuffer(errbuf, "%s: ", libpq_gettext(actx->errctx));
if (PQExpBufferDataBroken(actx->errbuf))
- appendPQExpBufferStr(&conn->errorMessage,
@@ src/interfaces/libpq-oauth/oauth-curl.c:
pg_fe_run_oauth_flow_impl(PGconn *conn)
if (actx->curl_err[0])
{
- size_t len;
-
+- size_t len;
+-
- appendPQExpBuffer(&conn->errorMessage,
- " (libcurl: %s)",
actx->curl_err);
+ appendPQExpBuffer(errbuf, " (libcurl: %s)", actx->curl_err);
@@ src/interfaces/libpq-oauth/oauth-curl.c:
pg_fe_run_oauth_flow_impl(PGconn *conn)
/* Sometimes libcurl adds a newline to the error buffer. :( */
- len = conn->errorMessage.len;
- if (len >= 2 && conn->errorMessage.data[len - 2] == '\n')
-+ len = errbuf->len;
-+ if (len >= 2 && errbuf->data[len - 2] == '\n')
++ if (errbuf->len >= 2 && errbuf->data[errbuf->len - 2] == '\n')
{
- conn->errorMessage.data[len - 2] = ')';
- conn->errorMessage.data[len - 1] = '\0';
- conn->errorMessage.len--;
-+ errbuf->data[len - 2] = ')';
-+ errbuf->data[len - 1] = '\0';
++ errbuf->data[errbuf->len - 2] = ')';
++ errbuf->data[errbuf->len - 1] = '\0';
+ errbuf->len--;
}
}
v9-0001-Add-minor-version-counterpart-to-PG_-MAJORVERSION.patch
Description: Binary data
v9-0002-oauth-Move-the-builtin-flow-into-a-separate-modul.patch
Description: Binary data
