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

["Đoàn Trần Công Danh" <congdanhqx@gmail.com>]

On 2021-04-18 10:08:31+0300, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> 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
> 

Yes, inline function are always better than macro.
I feel embarassed that I couldn't think about that earlier.

Here is a revised patch:
---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 an inline
function.

Helped-by: Tomi Ollila <tomi.ollila@iki.fi>
---
 compat/canonicalize_file_name.c |  2 +-
 compat/compat.h                 | 13 ++++++++-----
 lib/open.cc                     |  4 ++--
 notmuch-config.c                |  2 +-
 4 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/compat/canonicalize_file_name.c b/compat/canonicalize_file_name.c
index 000f9e78..84f2a2c2 100644
--- a/compat/canonicalize_file_name.c
+++ b/compat/canonicalize_file_name.c
@@ -4,7 +4,7 @@
 #include <stdlib.h>
 
 char *
-canonicalize_file_name (const char *path)
+notmuch_canonicalize_file_name (const char *path)
 {
 #ifdef PATH_MAX
     char *resolved_path =  malloc (PATH_MAX + 1);
diff --git a/compat/compat.h b/compat/compat.h
index 8f15e585..c3ef4051 100644
--- a/compat/compat.h
+++ b/compat/compat.h
@@ -37,12 +37,15 @@ extern "C" {
 #define _POSIX_PTHREAD_SEMANTICS 1
 #endif
 
-#if ! HAVE_CANONICALIZE_FILE_NAME
-/* we only call this function from C, and this makes testing easier */
-#ifndef __cplusplus
+#if HAVE_CANONICALIZE_FILE_NAME
+static inline char *
+notmuch_canonicalize_file_name (const char *path)
+{
+	return canonicalize_file_name (path);
+}
+#else
 char *
-canonicalize_file_name (const char *path);
-#endif
+notmuch_canonicalize_file_name (const char *path);
 #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);
-- 

Danh