Hi Tomi,

Thanks for your detailled review. Please see questions below.

Cheers,
Bijan

Tomi Ollila <tomi.ollila@iki.fi> schrieb am So., 8. Mai 2016 um 18:47 Uhr:

> On Sun, May 08 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.
>
> Thank you for your interest in improving notmuch!
>
> There are a few things that needs to be sorted out for this feature to be
> good:
>
> This implementation does not handle ~user/ prefix: i.e. home directory of
> 'user' (maybe this should not, but it should handle the case).
>

I don't get it. Is '~user" an alternative to '~'?

Whether or not ~user is handled, it should check that slash (/) follows...
>
>
So I guess you aim at the case where someone sets `path=~`? On the other
hand why is this checking not necessary in the "normal" case where no
expanding of `~` is done? Or is it maybe already handled in
`lib/database.cc`. Just to be clear I tested that it works currently with
`path=~/.mail`.


> IIRC there is some ready-made implementations of the above -- but if not,
> one option is to check how (expand-file-name) works in emacs for reference.
>
>
Well there is wordexp (http://linux.die.net/man/3/wordexp) but I wasn't
sure if I should use it. The getenv just seemed simpler but maybe it is
necessary.


>
> Something more inline:
>
>
> > ---
> > Not sure this is completely plattform independent.
>
> The implementation looked like it is platform independent -- at least on
> plattforms we care about...
>
> > I also don't know how to implement a unit test for this.
>
> I know... and I can do that if we get 1) decide that this feature will be
> supported and 2) decide how this feature should work and 3) someone(tm)
> does proper implementation ;)
>
> Tomi
>
> PS: still more to follow below.
>
>
> > ---
> >  notmuch-config.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/notmuch-config.c b/notmuch-config.c
> > index d252bb2..c9f26ef 100644
> > --- a/notmuch-config.c
> > +++ b/notmuch-config.c
> > @@ -605,7 +605,16 @@ _config_set_list (notmuch_config_t *config,
> >  const char *
> >  notmuch_config_get_database_path (notmuch_config_t *config)
> >  {
> > -    return _config_get (config, &config->database_path, "database",
> "path");
> > +    const char* path = _config_get (config, &config->database_path,
> "database", "path");
> > +    if (path != NULL && path[0] == '~') {
> > +        char *home_path = getenv("HOME");
> > +        char *shortened_path = malloc( sizeof(char) * ( strlen (path) -
> 1 ) );
> > +        strncpy(shortened_path, path + 2, strlen(path));
> > +        return talloc_asprintf (NULL, "%s/%s", home_path,
> shortened_path);
>
> In the implementation above matching free() for malloc() is not done -- but
> actually the malloc is unnecessary -- path + 2 could have been used there
> (after one checked that path[1] is '/' (provided that ~/ were the only
> thing we supported...))
>
> In strncpy() above length arg is strlen(path) but there is 1 byte less
> allocated in shortened_path -- if src arg in the above strncpy() were
> something else it could overwrite the allocated space by 2 bytes.
>
>
Yes, sorry. My C is a little rusty ;). I was happy when it worked. But when
we agree that wordexp is the way to go, this can be removed anyhow.


>
>
> > +    }
> > +    else {
> > +        return path;
> > +    }
> >  }
> >
> >  void
> > --
> > 1.9.1
> >
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > https://notmuchmail.org/mailman/listinfo/notmuch
>