2f/2daed5a44a94352f348afe291cc8b501874a9d

   1 Return-Path: <david@tethera.net>\r
   2 X-Original-To: notmuch@notmuchmail.org\r
   3 Delivered-To: notmuch@notmuchmail.org\r
   4 Received: from localhost (localhost [127.0.0.1])\r
   5  by arlo.cworth.org (Postfix) with ESMTP id 6EED66DE0AC2\r
   6  for <notmuch@notmuchmail.org>; Tue,  9 Feb 2016 04:57:24 -0800 (PST)\r
   7 X-Virus-Scanned: Debian amavisd-new at cworth.org\r
   8 X-Spam-Flag: NO\r
   9 X-Spam-Score: -0.308\r
  10 X-Spam-Level: \r
  11 X-Spam-Status: No, score=-0.308 tagged_above=-999 required=5 tests=[AWL=0.243,\r
  12   RP_MATCHES_RCVD=-0.55, SPF_PASS=-0.001] autolearn=disabled\r
  13 Received: from arlo.cworth.org ([127.0.0.1])\r
  14  by localhost (arlo.cworth.org [127.0.0.1]) (amavisd-new, port 10024)\r
  15  with ESMTP id NuYj2t_V0f9o for <notmuch@notmuchmail.org>;\r
  16  Tue,  9 Feb 2016 04:57:22 -0800 (PST)\r
  17 Received: from fethera.tethera.net (fethera.tethera.net [198.245.60.197])\r
  18  by arlo.cworth.org (Postfix) with ESMTPS id 3E0046DE02CE\r
  19  for <notmuch@notmuchmail.org>; Tue,  9 Feb 2016 04:57:21 -0800 (PST)\r
  20 Received: from remotemail by fethera.tethera.net with local (Exim 4.84)\r
  21  (envelope-from <david@tethera.net>)\r
  22  id 1aT7qP-0000H3-80; Tue, 09 Feb 2016 07:56:37 -0500\r
  23 Received: (nullmailer pid 10963 invoked by uid 1000);\r
  24  Tue, 09 Feb 2016 12:57:15 -0000\r
  25 From: David Bremner <david@tethera.net>\r
  26 To: Daniel Kahn Gillmor <dkg@fifthhorseman.net>,\r
  27  Notmuch Mail <notmuch@notmuchmail.org>\r
  28 Subject: Re: [PATCH v3 01/16] add util/search-path.{c,\r
  29  h} to test for executables in $PATH\r
  30 In-Reply-To: <1454272801-23623-2-git-send-email-dkg@fifthhorseman.net>\r
  31 References: <1454272801-23623-1-git-send-email-dkg@fifthhorseman.net>\r
  32  <1454272801-23623-2-git-send-email-dkg@fifthhorseman.net>\r
  33 User-Agent: Notmuch/0.21+5~gca076ce (http://notmuchmail.org) Emacs/24.5.1\r
  34  (x86_64-pc-linux-gnu)\r
  35 Date: Tue, 09 Feb 2016 08:57:15 -0400\r
  36 Message-ID: <87oabqvy6s.fsf@maritornes.cs.unb.ca>\r
  37 MIME-Version: 1.0\r
  38 Content-Type: multipart/mixed; boundary="=-=-="\r
  39 X-BeenThere: notmuch@notmuchmail.org\r
  40 X-Mailman-Version: 2.1.20\r
  41 Precedence: list\r
  42 List-Id: "Use and development of the notmuch mail system."\r
  43  <notmuch.notmuchmail.org>\r
  44 List-Unsubscribe: <https://notmuchmail.org/mailman/options/notmuch>,\r
  45  <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
  46 List-Archive: <http://notmuchmail.org/pipermail/notmuch/>\r
  47 List-Post: <mailto:notmuch@notmuchmail.org>\r
  48 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
  49 List-Subscribe: <https://notmuchmail.org/mailman/listinfo/notmuch>,\r
  50  <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
  51 X-List-Received-Date: Tue, 09 Feb 2016 12:57:24 -0000\r
  52 \r
  53 --=-=-=\r
  54 Content-Type: text/plain\r
  55 \r
  56 Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:\r
  57 \r
  58 > +notmuch_bool_t\r
  59 > +test_for_executable(const char* exename)\r
  60 > +{\r
  61 > +    char *c = NULL, *save = NULL, *tok;\r
  62 > +    size_t n;\r
  63 > +    int dfd = -1;\r
  64 \r
  65 c, dfd, and n could be more meaningful as variable names.\r
  66 \r
  67 % uncrustify --config devel/uncrustify.cfg --replace util/search-path.c util/search-path.h\r
  68 \r
  69 yields quite a few whitespace changes (diff attached)\r
  70 \r
  71 \r
  72 --=-=-=\r
  73 Content-Type: text/x-diff\r
  74 Content-Disposition: inline; filename=wschanges.diff\r
  75 \r
  76 diff --git a/util/search-path.c b/util/search-path.c\r
  77 index 5eac367..39601e4 100644\r
  78 --- a/util/search-path.c\r
  79 +++ b/util/search-path.c\r
  80 @@ -9,47 +9,47 @@\r
  81  \r
  82  \r
  83  notmuch_bool_t\r
  84 -test_for_executable(const char* exename)\r
  85 +test_for_executable (const char *exename)\r
  86  {\r
  87      char *c = NULL, *save = NULL, *tok;\r
  88      size_t n;\r
  89      int dfd = -1;\r
  90      notmuch_bool_t ret = FALSE;\r
  91  \r
  92 -    if (strchr(exename, '/')) {\r
  93 -       if (0 == access(exename, X_OK))\r
  94 +    if (strchr (exename, '/')) {\r
  95 +       if (0 == access (exename, X_OK))\r
  96             return TRUE;\r
  97         else\r
  98             return FALSE;\r
  99      }\r
 100 -    \r
 101 -    c = getenv("PATH");\r
 102 +\r
 103 +    c = getenv ("PATH");\r
 104      if (c)\r
 105 -       c = talloc_strdup(NULL, c);\r
 106 +       c = talloc_strdup (NULL, c);\r
 107      else {\r
 108 -       n = confstr(_CS_PATH, NULL, 0);\r
 109 -       c = (char*)talloc_size(NULL, n);\r
 110 -       if (!c)\r
 111 +       n = confstr (_CS_PATH, NULL, 0);\r
 112 +       c = (char *) talloc_size (NULL, n);\r
 113 +       if (! c)\r
 114             return FALSE;\r
 115 -       confstr(_CS_PATH, c, n);\r
 116 +       confstr (_CS_PATH, c, n);\r
 117      }\r
 118  \r
 119 -    tok = strtok_r(c, ":", &save);\r
 120 +    tok = strtok_r (c, ":", &save);\r
 121      while (tok) {\r
 122 -       dfd = open(tok, O_DIRECTORY | O_RDONLY);\r
 123 +       dfd = open (tok, O_DIRECTORY | O_RDONLY);\r
 124         if (dfd != -1) {\r
 125 -           if (!faccessat(dfd, exename, X_OK, 0)) {\r
 126 +           if (! faccessat (dfd, exename, X_OK, 0)) {\r
 127                 ret = TRUE;\r
 128                 goto done;\r
 129             }\r
 130 -           close(dfd);\r
 131 +           close (dfd);\r
 132         }\r
 133 -       tok = strtok_r(NULL, ":", &save);\r
 134 +       tok = strtok_r (NULL, ":", &save);\r
 135      }\r
 136 -done:\r
 137 +  done:\r
 138      if (dfd != -1)\r
 139 -       close(dfd);\r
 140 +       close (dfd);\r
 141      if (c)\r
 142 -       talloc_free(c);\r
 143 +       talloc_free (c);\r
 144      return ret;\r
 145  }\r
 146 diff --git a/util/search-path.h b/util/search-path.h\r
 147 index 727d0b3..14c4d14 100644\r
 148 --- a/util/search-path.h\r
 149 +++ b/util/search-path.h\r
 150 @@ -4,13 +4,13 @@\r
 151  #include "notmuch.h"\r
 152  \r
 153  /* can an executable be found with the given name?\r
 154 - * \r
 155 + *\r
 156   * Return TRUE only if we can find something to execute with the\r
 157   * associated name.\r
 158   *\r
 159   * if the name has a '/' in it, we look for it directly with\r
 160   * access(exename, X_OK).\r
 161 - * \r
 162 + *\r
 163   * otherwise, we look for it in $PATH (or in confstr(_CS_PATH), if\r
 164   * $PATH is unset).\r
 165   *\r
 166 @@ -19,6 +19,6 @@\r
 167   */\r
 168  \r
 169  notmuch_bool_t\r
 170 -test_for_executable(const char *exename);\r
 171 +test_for_executable (const char *exename);\r
 172  \r
 173  #endif\r
 174 \r
 175 --=-=-=\r
 176 Content-Type: text/plain\r
 177 \r
 178 \r
 179 > +    notmuch_bool_t ret = FALSE;\r
 180 > +\r
 181 > +    if (strchr(exename, '/')) {\r
 182 > +     if (0 == access(exename, X_OK))\r
 183 > +         return TRUE;\r
 184 > +     else\r
 185 > +         return FALSE;\r
 186 > +    }\r
 187 > +    \r
 188 > +    c = getenv("PATH");\r
 189 > +    if (c)\r
 190 > +     c = talloc_strdup(NULL, c);\r
 191 \r
 192 Is there some advantage to using the talloc_ functions here?\r
 193 \r
 194 > +    else {\r
 195 \r
 196 Is n needed outside this block? if not, it could be declared here (and\r
 197 then I don't care about the single letter name).\r
 198 \r
 199 > +     n = confstr(_CS_PATH, NULL, 0);\r
 200 \r
 201 according to a glance at the man page, this might return 0 if there is\r
 202 no value for _CS_PATH set?\r
 203 \r
 204 \r
 205 > +\r
 206 > +    tok = strtok_r(c, ":", &save);\r
 207 > +    while (tok) {\r
 208 same comment about block local declaration of dfd\r
 209 \r
 210 > +     dfd = open(tok, O_DIRECTORY | O_RDONLY);\r
 211 \r
 212 > +     tok = strtok_r(NULL, ":", &save);\r
 213 \r
 214 not sure if it helps, but there is also strtok_len in libutil\r
 215 \r
 216 > +done:\r
 217 \r
 218 as a real nitpick, we have always (?) used DONE for a label.\r
 219 \r
 220 --=-=-=--\r