From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by arlo.cworth.org (Postfix) with ESMTP id 6A5D06DE028C for ; Fri, 26 Aug 2016 03:57:13 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at cworth.org X-Spam-Flag: NO X-Spam-Score: -0.007 X-Spam-Level: X-Spam-Status: No, score=-0.007 tagged_above=-999 required=5 tests=[AWL=0.004, SPF_PASS=-0.001, T_RP_MATCHES_RCVD=-0.01] 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 CAOXrTRFtOhb for ; Fri, 26 Aug 2016 03:57:12 -0700 (PDT) Received: from fethera.tethera.net (fethera.tethera.net [198.245.60.197]) by arlo.cworth.org (Postfix) with ESMTPS id 717FE6DE0270 for ; Fri, 26 Aug 2016 03:57:12 -0700 (PDT) Received: from remotemail by fethera.tethera.net with local (Exim 4.84_2) (envelope-from ) id 1bdEos-0001SL-CZ; Fri, 26 Aug 2016 06:57:06 -0400 Received: (nullmailer pid 28818 invoked by uid 1000); Fri, 26 Aug 2016 10:57:06 -0000 From: David Bremner To: Erik Rybakken , Jani Nikula , notmuch@notmuchmail.org Subject: Re: [PATCH] Add option `hooks.path` for setting the directory of hooks. In-Reply-To: <20160824215430.ljugc3vevx4ve2gq@dell> References: <20160824163006.dzdvzjvkg5uxtrnh@dell> <87vayqnjr3.fsf@nikula.org> <20160824215430.ljugc3vevx4ve2gq@dell> User-Agent: Notmuch/0.22.1 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Fri, 26 Aug 2016 07:57:06 -0300 Message-ID: <87shtrol19.fsf@zancas.localnet> MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.22 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: Fri, 26 Aug 2016 10:57:13 -0000 Erik Rybakken writes: > Hi again, > > I implemented the option for hooks myself. The patch is included. Please > bear with me, this is my first contribution to notmuch (and my first > attempt to write C code). I tested the option, and it seems to work. > > Best, > Erik Welcome. You may find https://notmuchmail.org/contributing/ has some helpful hints, if you didn't already see it. In particular, your patch needs an "meaningful commit message". You seem to have mostly managed to follow the notmuch coding style, so thanks for that. > +General > +------- > + > +Add option `hooks.path` for setting the directory for hooks. If > +unset, it will default to the `.notmuch/hooks` sub-directory. > + As a minor point, this should go under "Command Line Interface", since hooks are strictly a CLI feature. > +static const char hooks_config_comment[] = > + " Hook configuration\n" > + "\n" > + " The only value supported here is 'path' which should be the directory\n" > + " where your hooks exists.\n"; > + "hooks exist." No real objections to the code or the feature, but you'll need to update the test suite. In particular T030-config and T040-setup are currently broken by your patch (but easy to fix). You should also add at least test to T400-hooks.sh to ensure running hooks from a non-standard location works. Feel free to ask for help with the test suite, if copying existing tests isn't enough. d