From 6ca8041a6e4dadb8791d6cf8780c359d23616776 Mon Sep 17 00:00:00 2001 From: Bijan Chokoufe Date: Sun, 8 May 2016 18:50:40 +0000 Subject: [PATCH] Re: [PATCH] config: Expand ~ to $HOME --- 36/88f450a2e3a51653783fb34e0268f5abe9f640 | 325 ++++++++++++++++++++++ 1 file changed, 325 insertions(+) create mode 100644 36/88f450a2e3a51653783fb34e0268f5abe9f640 diff --git a/36/88f450a2e3a51653783fb34e0268f5abe9f640 b/36/88f450a2e3a51653783fb34e0268f5abe9f640 new file mode 100644 index 000000000..1b26997c9 --- /dev/null +++ b/36/88f450a2e3a51653783fb34e0268f5abe9f640 @@ -0,0 +1,325 @@ +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-- -- 2.26.2