From: Tomi Ollila <tomi.ollila@iki.fi>
To: "Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
"David Bremner" <david@tethera.net>
Cc: notmuch@notmuchmail.org
Subject: Re: [PATCH] compat: expose canonicalize_file_name to C++
Date: Sun, 18 Apr 2021 10:08:31 +0300 [thread overview]
Message-ID: <m25z0koys0.fsf@guru.guru-group.fi> (raw)
In-Reply-To: <YHux8vY+2+cexMyL@danh.dev>
On Sun, Apr 18 2021, Đoàn Trần Công Danh wrote:
> On 2021-04-17 11:39:59-0300, David Bremner <david@tethera.net> wrote:
>> Đoàn Trần Công Danh <congdanhqx@gmail.com> writes:
>>
>> >
>> > However, I see that lib/open.cc uses g_key_file_get_value from GLib
>> > already, we may switch to g_canonicalize_file_name then?
>> >
>>
>> Yes that could work. I think the treatment of NULL input might need some
>> extra care with g_canonicalize_file_name; at least my 5 minute attempt
>> to do a replacement causes many test failures. Do you want to make a
>> patch that uses g_canonicalize_file_name? The one other place we use
>> canonicalize_file_name (not totally coincidentally) also uses glib, so
>> in principle we could completely eliminate the compat function.
>
> Now, I'm thinking more about it and digging into GLib history,
> I think using g_canonicalize_filename would be problem, since it's
> available from 2.57.1/2.58 only. I think we're requiring glib-2.0 2.22
> as of it's now. A big jump in dependencies isn't worth it.
>
> I think below patch would be better?
If that worked (I did not test), instead of macro "hackery", inline function
could be better(?) i.e. something like the following in notmuch_compat.h:
#if HAVE_CANONICALIZE_FILE_NAME
static inline char *
notmuch_canonicalize_file_name (const char *path)
{
return canonicalize_file_name (path)
}
#else
char *
notmuch_canonicalize_file_name (const char *path);
#endif
(OTOH I'd like to get upgraded to 2.57.1 but I still have 2.49 in the
system that builds this notmuch so...
# 2.57.1 works w/o meson/ninja (and has ./configure) but requires libmount
#glib_lnk=http://ftp.gnome.org/pub/gnome/sources/glib/2.57/glib-2.57.1.tar.xz
#glib_cks=d029e7c4536835f1f103472f7510332c28d58b9b7d6cd0e9f45c2653e670d9b4
glib_lnk=http://ftp.gnome.org/pub/gnome/sources/glib/2.49/glib-2.49.4.tar.xz
glib_cks=9e914f9d7ebb88f99f234a7633368a7c1133ea21b5cac9db2a33bc25f7a0e0d1
)
> -/* we only call this function from C, and this makes testing easier */
> -#ifndef __cplusplus
> char *
> -canonicalize_file_name (const char *path);
> -#endif
> +notmuch_canonicalize_file_name (const char *path);
> +#else
> +#define notmuch_canonicalize_file_name canonicalize_file_name
> #endif
>
> Anyway, I see some failure in the testsuite due to:
> - *My* hostname(1) (from coreutils) doesn't understand "-f"
> - All emacs tests depend on dtach(1) but the
> test_require_external_prereq dtach is missing. Would you want
> to see those patches for test_require_external_prereq dtach?
> It's pretty trivial.
> -----8<-----
> Subject: [PATCH] compat: rename {,notmuch_}canonicalize_file_name
>
> When compat canonicalize_file_name was introduced, it was limited to
> C code only because it was used by C code only during that time.
>
> From 5ec6fd4d, (lib/open: check for split configuration when creating
> database., 2021-02-16), lib/open.cc, which is C++, relies on the
> existent of canonicalize_file_name.
>
> However, we can't blindly enable canonicalize_file_name for C++ code,
> because different implementation has different additional signature for
> C++ and users can arbitrarily add -DHAVE_CANONICALIZE_FILE_NAME=0 to
> {C,CXX}FLAGS.
>
> Let's put our implementation of canonicalize_file_name to our namespace,
> and prefer system's canonicalize_file_name if it's existed via a macro.
> ---
> compat/canonicalize_file_name.c | 6 +++++-
> compat/compat.h | 7 +++----
> lib/open.cc | 4 ++--
> notmuch-config.c | 2 +-
> 4 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/compat/canonicalize_file_name.c b/compat/canonicalize_file_name.c
> index 000f9e78..ba003268 100644
> --- a/compat/canonicalize_file_name.c
> +++ b/compat/canonicalize_file_name.c
> @@ -3,8 +3,12 @@
> #undef _GNU_SOURCE
> #include <stdlib.h>
>
> +#ifdef notmuch_canonicalize_file_name
> +#undef notmuch_canonicalize_file_name
> +#endif
> +
> char *
> -canonicalize_file_name (const char *path)
> +notmuch_canonicalize_file_name (const char *path)
w> {
> #ifdef PATH_MAX
> char *resolved_path = malloc (PATH_MAX + 1);
> diff --git a/compat/compat.h b/compat/compat.h
> index 8f15e585..a38bc460 100644
> --- a/compat/compat.h
> +++ b/compat/compat.h
> @@ -38,11 +38,10 @@ extern "C" {
> #endif
>
> #if ! HAVE_CANONICALIZE_FILE_NAME
> -/* we only call this function from C, and this makes testing easier */
> -#ifndef __cplusplus
> char *
> -canonicalize_file_name (const char *path);
> -#endif
> +notmuch_canonicalize_file_name (const char *path);
> +#else
> +#define notmuch_canonicalize_file_name canonicalize_file_name
> #endif
>
> #if ! HAVE_GETLINE
> diff --git a/lib/open.cc b/lib/open.cc
> index 5d80a884..7454ffae 100644
> --- a/lib/open.cc
> +++ b/lib/open.cc
> @@ -612,9 +612,9 @@ notmuch_database_create_with_config (const char *database_path,
> _set_database_path (notmuch, database_path);
>
> if (key_file && ! split) {
> - char *mail_root = canonicalize_file_name (
> + char *mail_root = notmuch_canonicalize_file_name (
> g_key_file_get_value (key_file, "database", "mail_root", NULL));
> - char *db_path = canonicalize_file_name (database_path);
> + char *db_path = notmuch_canonicalize_file_name (database_path);
>
> split = (mail_root && (0 != strcmp (mail_root, db_path)));
>
> diff --git a/notmuch-config.c b/notmuch-config.c
> index 16e86916..d8d47768 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -327,7 +327,7 @@ notmuch_conffile_save (notmuch_conffile_t *config)
> }
>
> /* Try not to overwrite symlinks. */
> - filename = canonicalize_file_name (config->filename);
> + filename = notmuch_canonicalize_file_name (config->filename);
> if (! filename) {
> if (errno == ENOENT) {
> filename = strdup (config->filename);
> --
> 2.31.1
>
>
>
> --
> Danh
> _______________________________________________
> notmuch mailing list -- notmuch@notmuchmail.org
> To unsubscribe send an email to notmuch-leave@notmuchmail.org
next prev parent reply other threads:[~2021-04-18 7:08 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-17 0:18 [PATCH] compat: expose canonicalize_file_name to C++ Đoàn Trần Công Danh
2021-04-17 12:13 ` David Bremner
2021-04-17 12:49 ` Đoàn Trần Công Danh
2021-04-17 14:39 ` David Bremner
2021-04-18 4:13 ` Đoàn Trần Công Danh
2021-04-18 7:08 ` Tomi Ollila [this message]
2021-04-18 10:38 ` Đoàn Trần Công Danh
2021-04-23 17:31 ` David Bremner
2021-04-24 0:29 ` Đoàn Trần Công Danh
2021-04-18 12:48 ` David Bremner
2021-04-18 16:19 ` Tomi Ollila
2021-04-18 16:47 ` Tomi Ollila
2021-04-24 0:57 ` [PATCH v3] compat: rename {,notmuch_}canonicalize_file_name Đoàn Trần Công Danh
2021-04-24 1:01 ` Đoàn Trần Công Danh
2021-04-24 1:05 ` [PATCH v4] " Đoàn Trần Công Danh
2021-04-24 11:43 ` David Bremner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://notmuchmail.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=m25z0koys0.fsf@guru.guru-group.fi \
--to=tomi.ollila@iki.fi \
--cc=congdanhqx@gmail.com \
--cc=david@tethera.net \
--cc=notmuch@notmuchmail.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://yhetil.org/notmuch.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).