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 3A0346DE0C19 for ; Tue, 26 Sep 2017 12:11:15 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at cworth.org X-Spam-Flag: NO X-Spam-Score: 0.014 X-Spam-Level: X-Spam-Status: No, score=0.014 tagged_above=-999 required=5 tests=[AWL=0.034, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-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 PuLNsgLSvs6J for ; Tue, 26 Sep 2017 12:11:14 -0700 (PDT) Received: from mail-lf0-f41.google.com (mail-lf0-f41.google.com [209.85.215.41]) by arlo.cworth.org (Postfix) with ESMTPS id BB60A6DE0B78 for ; Tue, 26 Sep 2017 12:11:13 -0700 (PDT) Received: by mail-lf0-f41.google.com with SMTP id y187so5561300lfc.8 for ; Tue, 26 Sep 2017 12:11:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nikula-org.20150623.gappssmtp.com; s=20150623; h=from:to:subject:in-reply-to:references:date:message-id:mime-version; bh=DVuYBrNspBKTpTBHURcFXkoiUw7pIaoOhxE9L+GVxaI=; b=G/GJH8fupeQ5uZrkmmsonUhDmIBrWl7FlOYvdGjgOFKE1mW2trGZMEet1uIR+X/NvE batGDbosfIOPO7xqgDdBY3+FBPgNgBIissZqUZmj8mXNzEn0ymYhKdXjMhmuyPrujteL uGgbmrgkfn2ehLQJkn5i8fF8KVtaEGkaXc0J6C2BWil+8Ess/BoeBlc4Bh+bygqbke1u Osar2Oy+tgxz8HfrhkD8cR2BuE4H+Wr07p/pxUQKQ1kS2e9eY3L9Y2gSocIrOaH/FkpH QClolEv4wTaeNS77c5060ip974uyfYU5yYgB+G40rS1RQcXjBuTg8Ntic/LGR0tRE/zT 4xsA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:in-reply-to:references:date :message-id:mime-version; bh=DVuYBrNspBKTpTBHURcFXkoiUw7pIaoOhxE9L+GVxaI=; b=F+/sUZIViRscFqdGBJnfYRK/Cko4DZxti4b0FletUIN6Xf5rA+VclC2FpAOUXHhruo WVXxTGjTF4+pXGl/ZxmrmiDCYilQdtXxRHLESp54Q3H1sgbept04s0k4ape++j5j7cUn GxKnYDIv4uEUnQ3QbkcHv98MD1oZ6I01UXd1XPd/k3wD6LhkRSsg4WilFKdPCowuFMKR yf13nNaG5Mir6U4FyXXQhdyoo3g1cDIasjiLvSpXIjYw8sKuzf5VTnsRa3QmDwgnc9OI Aa8XKkqqc1gy3WolN26OKkYc7yB6H3VaKmwQuK8UbYq9dQ+MPrdXvEB0NO90B1qb2QZb aM7g== X-Gm-Message-State: AHPjjUhtBISaheRUphDVnQ2G7PyEB3v2RW9ACYkSCvbxxUAk8VjJABfZ Au1oq9zXU7ON15Iw9NDAM6cMpYjSD6w= X-Google-Smtp-Source: AOwi7QBeND3j7aiHBdP9Qf0LCuTcKYdYOLojHjJQZRtw3Kzv51HIbH0u1aegibEbC1GzJ5N+idWEMw== X-Received: by 10.25.147.28 with SMTP id v28mr4100434lfd.177.1506453071937; Tue, 26 Sep 2017 12:11:11 -0700 (PDT) Received: from localhost (mobile-access-5d6a60-234.dhcp.inet.fi. [93.106.96.234]) by smtp.gmail.com with ESMTPSA id t125sm1423264lfe.73.2017.09.26.12.11.08 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 26 Sep 2017 12:11:10 -0700 (PDT) From: Jani Nikula To: David Bremner , notmuch@notmuchmail.org Subject: Re: [PATCH 5/6] cli/new: support // in new.ignore In-Reply-To: <87zi9i437c.fsf@tethera.net> References: <5e252ac400993a2fecdbe8c0d739aa4d23d3de72.1504280923.git.jani@nikula.org> <87zi9i437c.fsf@tethera.net> Date: Tue, 26 Sep 2017 22:11:05 +0300 Message-ID: <87tvzp46nq.fsf@nikula.org> MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.23 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: Tue, 26 Sep 2017 19:11:15 -0000 On Mon, 25 Sep 2017, David Bremner wrote: > Jani Nikula writes: > >> + A regular expression delimited with // that will be matched >> + against the path of the file or directory relative to the >> + database path. The beginning and end of string must be >> + explictly anchored. For example, /.*/foo$/ would match >> + "bar/foo" and "bar/baz/foo", but not "foo" or "bar/foobar". > > Is it worth remarking that '/' does not need to be escaped? or more > interestingly, what happens if it is escaped, do things break? It just gets passed down to regcomp() with the escape and all. I'm not sure it's worth trying to exhaustively explain everything. >> >> +static notmuch_bool_t >> +_setup_ignore (notmuch_config_t *config, add_files_state_t *state) >> +{ > > Would be nice to document what this return value means. Something like: /* Jani forgot to do anything useful with the return value */ I think it was originally meant to return false on illegal input (e.g. opening '/' without closing '/') or regcomp() failing, but then I opted for the more lax approach. xregcomp() warns about failures though. > >> + const char **ignore_list, **ignore; >> + int nregex = 0, nverbatim = 0; >> + const char **verbatim = NULL; >> + regex_t *regex = NULL; >> + >> + ignore_list = notmuch_config_get_new_ignore (config, NULL); >> + if (! ignore_list) >> + return TRUE; >> + >> + for (ignore = ignore_list; *ignore; ignore++) { >> + const char *s = *ignore; >> + size_t len = strlen (s); >> + >> + if (len > 2 && s[0] == '/' && s[len - 1] == '/') { > > One thing we eventually settled on in the query parser is that an > opening '/' without a trailing '/' is an errror. But perhaps it's fine > to take a more permissive approach here. I'm fine either way, I just chose to be permissive. So do I make the function void and drop the return values, or make it check and return errors? > >> + >> + if (! state->ignore_regex_length) >> + return FALSE; > > It's a nitpick, even by the standards of this review, but I'd prefer an > explicit '> 0' check. ITYM (state->ignore_regex_length == 0) but ack. BR, Jani.