Return-Path: X-Original-To: notmuch@notmuchmail.org Delivered-To: notmuch@notmuchmail.org Received: from localhost (localhost [127.0.0.1]) by arlo.cworth.org (Postfix) with ESMTP id BE0B76DE00EB for ; Sun, 8 May 2016 11:50:59 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at cworth.org X-Spam-Flag: NO X-Spam-Score: -0.469 X-Spam-Level: X-Spam-Status: No, score=-0.469 tagged_above=-999 required=5 tests=[FREEMAIL_FORGED_FROMDOMAIN=0.249, FREEMAIL_FROM=0.001, HEADER_FROM_DIFFERENT_DOMAINS=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_PASS=-0.001] autolearn=disabled Received: from arlo.cworth.org ([127.0.0.1]) by localhost (arlo.cworth.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id acRG5CZCUHQM for ; Sun, 8 May 2016 11:50:51 -0700 (PDT) Received: from mail-ig0-f181.google.com (mail-ig0-f181.google.com [209.85.213.181]) by arlo.cworth.org (Postfix) with ESMTPS id CEF9C6DE0022 for ; Sun, 8 May 2016 11:50:50 -0700 (PDT) Received: by mail-ig0-f181.google.com with SMTP id bi2so87794004igb.0 for ; Sun, 08 May 2016 11:50:50 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=NGDpxcj6sKGZlBV7OaUWyVbyrUYcGEli/LR2N/qX1cM=; b=XWnRhrTbm6SMl2nrv2nv6WRY956f1MFOuNknjPh2cJlNdhBtQWTlPq150f7oFmJpKz 92qPlWPi/N+815BrmTe+IC4rAOOUInqDF0AUPWFYmCTZM3lyV7ntieFPPuT2BUx6V6QO sAzhxoyKFx0wIflhxHiV/a+6iVpl++Xl1EDnKb1/EsdM8VAuWnaKY6Mao7ARpyPEMAg9 UQfH9lNE9YWv9COvAvJh2jHkSM7oC0WAqr9Kj6UyFDIyEBACZMt3osu0f4Xwx9ieWVQ+ Mh7B6nnvY5qvfS4VrOtFaoT0leZkA4+nVvmaWzFj1Klxl9rIGABsPbpS7Ittjf3HRsjt fhog== X-Gm-Message-State: AOPr4FWWQPlt3k9LdRJtjrXS2oECufqS/9vZSTE94CDWMGLZhWiHE7pR/XuBsbSfuKRtiOwgiKbnPAwyU4SHig== X-Received: by 10.50.16.163 with SMTP id h3mr7451329igd.49.1462733449943; Sun, 08 May 2016 11:50:49 -0700 (PDT) MIME-Version: 1.0 References: <1462722574-4176-1-git-send-email-bijan@chokoufe.com> In-Reply-To: From: Bijan Chokoufe Date: Sun, 08 May 2016 18:50:40 +0000 Message-ID: Subject: Re: [PATCH] config: Expand ~ to $HOME To: Tomi Ollila , notmuch@notmuchmail.org Content-Type: multipart/alternative; boundary=f46d04426cceaf612e0532592b3d X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 08 May 2016 18:50:59 -0000 --f46d04426cceaf612e0532592b3d Content-Type: text/plain; charset=UTF-8 Hi Tomi, Thanks for your detailled review. Please see questions below. Cheers, Bijan Tomi Ollila schrieb am So., 8. Mai 2016 um 18:47 Uhr: > On Sun, May 08 2016, Bijan Chokoufe Nejad 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 > --f46d04426cceaf612e0532592b3d Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Tomi,

Thanks for your detailled revi= ew. Please see questions below.

Cheers,
= Bijan

Tomi Ollila <tomi.ollila@iki.fi> schrieb am S= o., 8. Mai 2016 um 18:47=C2=A0Uhr:
= 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).=C2=A0
=C2=A0
<= div>I don't get it. Is '~user" an alternative to '~'?<= /div>

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

So I guess you aim at the case where s= omeone sets `path=3D~`? On the other hand why is this checking not necessar= y in the "normal" case where no expanding of `~` is done? Or is i= t maybe already handled in `lib/database.cc`. Just to be clear I tested tha= t it works currently with `path=3D~/.mail`.
=C2=A0
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 m= aybe it is necessary.
=C2=A0

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.


> ---
>=C2=A0 notmuch-config.c | 11 ++++++++++-
>=C2=A0 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,
>=C2=A0 const char *
>=C2=A0 notmuch_config_get_database_path (notmuch_config_t *config)
>=C2=A0 {
> -=C2=A0 =C2=A0 return _config_get (config, &config->database_pa= th, "database", "path");
> +=C2=A0 =C2=A0 const char* path =3D _config_get (config, &config-&= gt;database_path, "database", "path");
> +=C2=A0 =C2=A0 if (path !=3D NULL && path[0] =3D=3D '~'= ;) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 char *home_path =3D getenv("HOME&quo= t;);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 char *shortened_path =3D malloc( sizeof(c= har) * ( strlen (path) - 1 ) );
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 strncpy(shortened_path, path + 2, strlen(= path));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 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 o= nly
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 g= o, this can be removed anyhow.
=C2=A0


> +=C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return path;
> +=C2=A0 =C2=A0 }
>=C2=A0 }
>
>=C2=A0 void
> --
> 1.9.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@n= otmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuc= h
--f46d04426cceaf612e0532592b3d--