Re: [PATCH] compat: expose canonicalize_file_name to C++

Subject: Re: [PATCH] compat: expose canonicalize_file_name to C++

Date: Sun, 18 Apr 2021 10:08:31 +0300

To: Đoàn Trần Công Danh, David Bremner

Cc: notmuch@notmuchmail.org

From: Tomi Ollila


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
_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org

Thread: