Date: Sun, 12 Apr 2020 13:53:40 +0200 (CEST)
From: Fabien COELHO <coelho@cri.ensmp.fr>
To: Justin Pryzby <pryzby@telsasoft.com>
cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
    David Steele <david@pgmasters.net>, pgsql-hackers@postgresql.org,
    "Bossart, Nathan" <bossartn@amazon.com>,
    Thomas Munro <thomas.munro@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: pg_ls_tmpdir to show directories and shared filesets (and
 pg_ls_*)
In-Reply-To: <20200331200812.GI14618@telsasoft.com>
Message-ID: <alpine.DEB.2.21.2004121145190.5257@pseudo>
References: <20200316214806.GQ26184@telsasoft.com>
 <20200316221736.GA15635@alvherre.pgsql>
 <20200317031427.GU26184@telsasoft.com>
 <alpine.DEB.2.21.2003170828250.17584@pseudo>
 <20200317190401.GY26184@telsasoft.com> <20200331200812.GI14618@telsasoft.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; format=flowed; charset=US-ASCII
Precedence: bulk


Hello Justin,

About v15, seen as one patch.

Patches serie applies cleanly, compiles, "make check" ok.

Documentation:
  - indent documentation text around 80 cols, as done around?
  - indent SQL example for readability and capitalize keywords
    (pg_ls_dir_metadata)
  - "For each file in a directory, list the file and its metadata."
    maybe: "List files and their metadata in a directory"?

Code:
  - Most pg_ls_*dir* functions call pg_ls_dir_files(), which looks like
    reasonable refactoring, ISTM that the code is actually smaller.
  - please follow pg style, eg not "} else {"
  - there is a "XXX" (meaning fixme?) tag remaining in a comment.
  - file types: why not do block & character devices, fifo and socket
    as well, before the unkown case?
  - I'm wondering whether could pg_stat_file call pg_ls_dir_files without
    too much effort? ISTM that the output structure nearly the same. I do
    not like much having one function specialized for files and one for
    directories.

Tests:
  - good, there are some!
  - indent SQL code, eg by starting a new line on new clauses?
  - put comments on separate lines (I'm not against it on principle, I do
    that, but I do not think that it is done much in test files).

-- 
Fabien.