From: Tomi Ollila <tomi.ollila@iki.fi>
To: Bijan Chokoufe Nejad <bijan@chokoufe.com>, notmuch@notmuchmail.org
Subject: Re: [PATCH v3] config: Expand ~ to $HOME
In-Reply-To: <1463238949-22612-1-git-send-email-bijan@chokoufe.com>
References: <1463238949-22612-1-git-send-email-bijan@chokoufe.com>
User-Agent: Notmuch/0.22+26~g2adc7d1 (http://notmuchmail.org) Emacs/24.5.1
 (x86_64-unknown-linux-gnu)
Date: Wed, 25 May 2016 18:55:43 +0300
Message-ID: <m2fut6cetc.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Sat, May 14 2016, Bijan Chokoufe Nejad <bijan@chokoufe.com> wrote:

> Very useful in case you want to keep your .notmuch-config synchronized across
> machines where you have different user names.


Good progress! There are quite a few issues still to resolve...

First, commit message (1st line got a bit outdated). More comments inline

>
> ---
> v2: Check for '~/' instead of checking for '~' and fix memory issue.
>     Unit test is still pending
> v3: Extended implementation handling also '~foo/' and '~foo'
> ---
>  notmuch-config.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/notmuch-config.c b/notmuch-config.c
> index 01bb185..25668a6 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -602,9 +602,55 @@ _config_set_list (notmuch_config_t *config,
>      *config_var = NULL;
>  }
>  
> +static char *
> +_config_get_user_folder (notmuch_config_t *config, const char *path) {

Naming, but that will go away via other future changes.

> +    char* tmp = talloc_strdup (config, path + 1);
> +    const char* user;
> +    if (strpbrk(tmp, "/") != NULL) {
> +      user = strtok (tmp, "/");

Above (sh|c)ould be done like:

  char *user = talloc_strdup (config, path + 1); // XXX path + 1
  char *tmp = strchr(user, '/');
  if (tmp) *tmp = '\0';

> +    }
> +    else {
> +      user = tmp;
> +    }
> +    struct passwd * pwd = getpwnam (user);
> +
> +    if (pwd == NULL) {
> +      printf("Config: could not expand %s.\n", path);
> +      printf("'%s' doesn't seem to be a valid user on this system.\n", user);
> +      exit(EXIT_FAILURE);
> +    }

Unable to expand ~ expression is not failure. it is just used unexpanded.
This is the way e.g. shell and emacs does it.

> +
> +    tmp = talloc_asprintf (config, "%s/%s", pwd->pw_dir, path + strlen (user) + 2);
> +    return tmp;
> +}
> +
> +static void
> +_config_expand_path (notmuch_config_t *config) {
> +    char* path = (char*) _config_get (config, &config->database_path,
> +        "database", "path");

_config_get done twice in all cases, which doesn't sound like a good idea.

> +    if (path != NULL && path[0] == '~') {
> +      char *new_path;
> +      if (strlen (path) == 1 || path[1] == '/') {

strlen(path) scans the whole string and then the return value is just
compared with 1 
better do path[1] == '\0' (after it was checked that path[0] != '\0' above)

> +        const char *home_path = getenv("HOME");
> +        if (strlen (path) == 1)

ditto

> +          new_path = talloc_asprintf (config, "%s%s", home_path, path + 1);
> +        else if (path[1] == '/')
> +          new_path = talloc_asprintf (config, "%s/%s", home_path, path + 2);
> +      }
> +      else {
> +        new_path = _config_get_user_folder (config, path);
> +      }
> +      notmuch_config_set_database_path (config, new_path);

I think this was my suggestion (or something similar) .. but that is not a
good idea -- some (unrelated) config setting will (or may) expand ~ and
write it to the config file.

> +      talloc_free (new_path);
> +    }
> +}
> +
>  const char *
>  notmuch_config_get_database_path (notmuch_config_t *config)
>  {
> +    // ideally a get_database_path should only get the database path and this
> +    // call to _config_expand_path would be done in a setup phase
> +    _config_expand_path (config);
>      return _config_get (config, &config->database_path, "database", "path");
>  }
>  
> -- 
> 2.7.4

I think there are at least these 2 ways that might get this done...

... both uses the expand_tilde WIP i did post earlier -- there is still
something to be fixed there (in addition to header file)

;; This buffer is for notes you don't want to save, and for Lisp evaluation.
;; If you want to create a file, visit that file with C-x C-f,
;; then enter the text in that file's own buffer.

--------------- either just with changes in notmuch_config_get_database_path

const char *
notmuch_config_get_database_path (notmuch_config_t *config)
{
    if (config->database_path != NULL)
        return config->database_path;

    (void)_config_get (config, &config->database_path, "database", "path");

    if (!!config->database_path && config->database_path[0] == '~') {
        const char * rest;
        const char * path = expand_tilde (config->database_path, &rest);
        if (path != NULL) {
            talloc_free (config->database_path);
            //config->database_path = [path stringByAppendingString rest];
            config->database_path = talloc_asprintf ("%s%s", path, rest);
        }
    }
    return config->database_path;
}

--------------- or with new helper _config_get_expand_tilde and using that...

const char *
notmuch_config_get_database_path (notmuch_config_t *config)
{
    return _config_get_expand_tilde (config, &config->database_path,
                                     "database", "path");
}


--- this is to be next to _config_get and the changes of these two needs to
--- be tracked carefully (if there is ever need to change); alternatively
--- support function and flag variable to decice when to expand and when not

static const char *
_config_get_expand_tilde (notmuch_config_t *config, char **field,
                          const char *group, const char *key)
{
    /* read from config file and cache value, if not cached already */
    if (*field == NULL) {
        char *value;
        value = g_key_file_get_string (config->key_file, group, key, NULL);
        if (value) {
            const char *path, *rest;
            if (value[0] == '~' && (path = expand_tilde (value, &rest)) != NULL)
                *field = talloc_asprintf ("%s%s", path, rest);
            else
                *field = talloc_strdup (config, value);
            free (value);
        }
    }
    return *field;
}



Tomi