[PATCH] config: Expand ~ to $HOME

[PATCH] config: Expand ~ to $HOME

[Bijan Chokoufe Nejad]

Very useful in case you want to keep your .notmuch-config synchronized across
machines where you have different user names.
---
Not sure this is completely plattform independent.
I also don't know how to implement a unit test for this.
---
 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);
+    }
+    else {
+        return path;
+    }
 }
 
 void
-- 
1.9.1

Re: [PATCH] config: Expand ~ to $HOME

[Tomi Ollila]

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).

Whether or not ~user is handled, it should check that slash (/) follows...

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.


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.



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

Re: [PATCH] config: Expand ~ to $HOME

[Bijan Chokoufe]

[-- Attachment #1: Type: text/plain, Size: 3693 bytes --]

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
>

[-- Attachment #2: Type: text/html, Size: 5457 bytes --]

Re: [PATCH] config: Expand ~ to $HOME

[Tomi Ollila]

On Sun, May 08 2016, Bijan Chokoufe <bijan@chokoufe.com> wrote:

> 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 '~'?

~user is ~ in case you're 'user' -- except that now that I think of it
~user could read home directory from /etc/passwd and not using $HOME.
If you're 'eve', then ~alice should definitely be different than ~

>
>> 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`.

your code checked that path[0] == '~', but nothing else, e.g.

~123randomstuff/... would expand as /home/user23randomstuff/... 

... and you have good point path being set as single '~' !

>> 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.

For the time being we could simply do checking that path[0] == '~' and
path[1] == '/' and then do expansion based on getenv ("HOME") -- and
comment that ~any_user is just not supported there.

From testing point of view doing one positive test which ensures that when
HOME is a string pointing to valid directory (and does not end with
trailing slash as it usually is) and path in configuration starts with '~/'
(and does not have multiple slashes following) works as expected...

(as a special case, from softare functionality point I don't see problem
setting path=~/ but from usability point that might not be the best -- but
some users may have peculiar preferences... ;D)

Tomi

Re: [PATCH] config: Expand ~ to $HOME

[Tomi Ollila]

On Mon, May 09 2016, Tomi Ollila <tomi.ollila@iki.fi> wrote:

> your code checked that path[0] == '~', but nothing else, e.g.
>
> ~123randomstuff/... would expand as /home/user23randomstuff/...

actually /home/user/23randomstuff/...

Tomi

Re: [PATCH] config: Expand ~ to $HOME

[Bijan Chokoufe Nejad]

[-- Attachment #1: Type: text/plain, Size: 3137 bytes --]

On 16-05-09, Tomi Ollila wrote:
> On Sun, May 08 2016, Bijan Chokoufe <bijan@chokoufe.com> wrote:
> 
> > 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 '~'?
> 
> ~user is ~ in case you're 'user' -- except that now that I think of it
> ~user could read home directory from /etc/passwd and not using $HOME.
> If you're 'eve', then ~alice should definitely be different than ~

OK I see. I never used ~user instead of ~ and don't see any advantage in using
~user but good to know it's there.

> 
> >
> >> 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`.
> 
> your code checked that path[0] == '~', but nothing else, e.g.
> 
> ~123randomstuff/... would expand as /home/user23randomstuff/... 
> 
> ... and you have good point path being set as single '~' !
> 
> >> 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.
> 
> For the time being we could simply do checking that path[0] == '~' and
> path[1] == '/' and then do expansion based on getenv ("HOME") -- and
> comment that ~any_user is just not supported there.

alright, done. I am still a bit confused by the way you do pull request here.
I am attaching the rebased commit as git patch. Is this correct or should I use
git send-email again? How do you keep track of lengthy PRs with this workflow?

Cheers,
Bijan

> 
> From testing point of view doing one positive test which ensures that when
> HOME is a string pointing to valid directory (and does not end with
> trailing slash as it usually is) and path in configuration starts with '~/'
> (and does not have multiple slashes following) works as expected...
> 
> (as a special case, from softare functionality point I don't see problem
> setting path=~/ but from usability point that might not be the best -- but
> some users may have peculiar preferences... ;D)
> 
> Tomi

[-- Attachment #2: v2-0001-config-Expand-to-HOME.patch --]
[-- Type: text/x-diff, Size: 1261 bytes --]

From ca6a86c6456d7fce2f18da8ec92d6d7ff14dc5f8 Mon Sep 17 00:00:00 2001
From: Bijan Chokoufe Nejad <bijan@chokoufe.com>
Date: Sun, 8 May 2016 15:47:50 +0200
Subject: [PATCH v2] config: Expand ~ to $HOME

Very useful in case you want to keep your .notmuch-config synchronized across
machines where you have different user names.

---
Check for '~/' instead of checking for '~' and fix memory issue.

Unit test is still pending
---
 notmuch-config.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index d252bb2..e97d5b0 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -605,7 +605,15 @@ _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");
+    /* '~user/some/path' is not supported but only '~/some/path' */
+    if (path != NULL && path[0] == '~' && path[1] == '/') {
+        char *home_path = getenv("HOME");
+        return talloc_asprintf (NULL, "%s/%s", home_path, path + 2);
+    }
+    else {
+        return path;
+    }
 }
 
 void
-- 
1.9.1

Re: [PATCH] config: Expand ~ to $HOME

[David Edmondson]

On Mon, May 09 2016, Bijan Chokoufe Nejad wrote:

>> ~user is ~ in case you're 'user' -- except that now that I think of it
>> ~user could read home directory from /etc/passwd and not using $HOME.
>> If you're 'eve', then ~alice should definitely be different than ~
>
> OK I see. I never used ~user instead of ~ and don't see any advantage in using
> ~user but good to know it's there.

A solution that supports ~ but not ~user seems incomplete.

Re: [PATCH] config: Expand ~ to $HOME

[Bijan Chokoufe]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

so what would be the spec for handling ~user?
As Tomi pointed out ~foo will point to different folders if set by user
'foo' or by user 'bar'.
To what folder should it point and where do I get this information?

David Edmondson <dme@dme.org> schrieb am Di., 10. Mai 2016 um 10:22 Uhr:

> On Mon, May 09 2016, Bijan Chokoufe Nejad wrote:
>
> >> ~user is ~ in case you're 'user' -- except that now that I think of it
> >> ~user could read home directory from /etc/passwd and not using $HOME.
> >> If you're 'eve', then ~alice should definitely be different than ~
> >
> > OK I see. I never used ~user instead of ~ and don't see any advantage in
> using
> > ~user but good to know it's there.
>
> A solution that supports ~ but not ~user seems incomplete.
>

[-- Attachment #2: Type: text/html, Size: 1111 bytes --]

Re: [PATCH] config: Expand ~ to $HOME

[David Edmondson]

On Wed, May 11 2016, Bijan Chokoufe wrote:

> so what would be the spec for handling ~user?

Look up "user" in the password file and replace "~user" with their home
directory. See getpwnam().

> As Tomi pointed out ~foo will point to different folders if set by user
> 'foo' or by user 'bar'.

~foo means "the home directory of user foo", independent of who is using
it, so it will be the same when used by both user foo and user bar.

> To what folder should it point and where do I get this information?
>
> David Edmondson <dme@dme.org> schrieb am Di., 10. Mai 2016 um 10:22 Uhr:
>
>> On Mon, May 09 2016, Bijan Chokoufe Nejad wrote:
>>
>> >> ~user is ~ in case you're 'user' -- except that now that I think of it
>> >> ~user could read home directory from /etc/passwd and not using $HOME.
>> >> If you're 'eve', then ~alice should definitely be different than ~
>> >
>> > OK I see. I never used ~user instead of ~ and don't see any advantage in
>> using
>> > ~user but good to know it's there.
>>
>> A solution that supports ~ but not ~user seems incomplete.
>>

Re: [PATCH] config: Expand ~ to $HOME

[Tomi Ollila]

On Wed, May 11 2016, David Edmondson <dme@dme.org> wrote:

> On Wed, May 11 2016, Bijan Chokoufe wrote:
>
>> so what would be the spec for handling ~user?
>
> Look up "user" in the password file and replace "~user" with their home
> directory. See getpwnam().
>
>> As Tomi pointed out ~foo will point to different folders if set by user
>> 'foo' or by user 'bar'.
>
> ~foo means "the home directory of user foo", independent of who is using
> it, so it will be the same when used by both user foo and user bar.
>

3 things more if you're cooking new patch

1) use git send-email -- then it is easier to apply.

2) on talloc() use config instead of NULL to lessen unfreed memory
   at program exit

3) talloc_free() old config->database_path and set it to point the new
   value.

Tomi



>> To what folder should it point and where do I get this information?
>>
>> David Edmondson <dme@dme.org> schrieb am Di., 10. Mai 2016 um 10:22 Uhr:
>>
>>> On Mon, May 09 2016, Bijan Chokoufe Nejad wrote:
>>>
>>> >> ~user is ~ in case you're 'user' -- except that now that I think of it
>>> >> ~user could read home directory from /etc/passwd and not using $HOME.
>>> >> If you're 'eve', then ~alice should definitely be different than ~
>>> >
>>> > OK I see. I never used ~user instead of ~ and don't see any advantage in
>>> using
>>> > ~user but good to know it's there.
>>>
>>> A solution that supports ~ but not ~user seems incomplete.
>>>