On Wed, Jan 7, 2026 at 3:33 PM albert tan <[email protected]> wrote: > Hi Join, > Thanks for pointing out extra issues. I have addressed them in v2.
Thanks, but I can't use this -- it looks like you just picked the first difference you saw in my proposed edits and ignored the rest, leaving the result just as awkward or ungrammatical as before (see below). That doesn't save me any time. > >/* > > * The directory parameter can be omitted, absolute, or relative to the > > * installation's base directory, which can be the sharedir or a custom > > * path that it was set extension_control_path. It depends where the > > * .control file was found. > > */ > > > >I imagine this means "or a custom path that was set via > >extension_control_path". - * path that it was set extension_control_path. It depends where the + * path that was set extension_control_path. It depends where the And, while looking again, there's another error here: "depends on" > >/* > > * Return a list of directories declared on extension_control_path GUC. > > */ > > >I would guess this normally phrased something like "Return the list of > >directories in extension_control_path". - * Return a list of directories declared on extension_control_path GUC. + * Return the list of directories declared on extension_control_path GUC. My main gripe here was "declared on <foo> GUC", not the article, which on second thought may as well remain "a list" since we do use that elsewhere to mean "an instance of the List type". I went with "Return a list of directories declared in the extension_control_path GUC.". Also, I decided that the paragraph that started this report needs to be rewritten: > ' The control file will be search on Extension_control_path paths if > control->control_dir is NULL, otherwise it will use the value of control_dir > to read and parse the .control file, so it assume that the control_dir is a > valid path for the control file being parsed.' - I'm not sure what the "assume" phrase is explaining, since we always check for errors when opening a file. The contract elsewhere for filling in control_dir doesn't seem as relevant as the NULL behavior. - Passive voice is often used to maintain discourse cohesion, but the following "it" doesn't refer to anything in particular. On Fri, Jan 9, 2026 at 1:17 PM li carol <[email protected]> wrote: > This should be corrected to: > > "Works in a very similar way to find_in_path, but it receives an already > parsed List of paths to search the basename, and it does not support macro > replacement or custom error messages (for simplicity). By 'already parsed > List of paths' this function expects that the paths already have all macros > replaced." Thanks, but I see more that should be done here (also, for something this long it's better to just mention what the corrections are, or write a patch, so I don't have to figure it out). The original comment is awkwardly written in that it has a separate paragraph to explain what a term in the first paragraph means, so I simplified it to say what it means. This has now gone beyond a trivial fix, so I've attached what I propose to commit barring other opinions. -- John Naylor Amazon Web Services
From 8c3c84f8b12d9bb845bb92cf8a6fdce338b20fee Mon Sep 17 00:00:00 2001 From: John Naylor <[email protected]> Date: Fri, 9 Jan 2026 11:02:32 +0700 Subject: [PATCH v4] Improve some comment wording and grammar in extension.c Noted while looking at reports of grammatical errors Reported-by: albert tan <[email protected]> Reported-by: Yuan Li(carol) <[email protected]> Discussion: https://postgr.es/m/CAEzortnJB7aue6miGT_xU2KLb3okoKgkBe4EzJ6yJ%3DY8LMB7gw%40mail.gmail.com --- src/backend/commands/extension.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c index bfabcfc579f..596105ee078 100644 --- a/src/backend/commands/extension.c +++ b/src/backend/commands/extension.c @@ -377,7 +377,7 @@ is_extension_script_filename(const char *filename) } /* - * Return a list of directories declared on extension_control_path GUC. + * Return a list of directories declared in the extension_control_path GUC. */ static List * get_extension_control_directories(void) @@ -454,9 +454,9 @@ get_extension_control_directories(void) } /* - * Find control file for extension with name in control->name, looking in the - * path. Return the full file name, or NULL if not found. If found, the - * directory is recorded in control->control_dir. + * Find control file for extension with name in control->name, looking in + * available paths. Return the full file name, or NULL if not found. + * If found, the directory is recorded in control->control_dir. */ static char * find_extension_control_filename(ExtensionControlFile *control) @@ -490,7 +490,7 @@ get_extension_script_directory(ExtensionControlFile *control) /* * The directory parameter can be omitted, absolute, or relative to the * installation's base directory, which can be the sharedir or a custom - * path that it was set extension_control_path. It depends where the + * path that was set via extension_control_path. It depends on where the * .control file was found. */ if (!control->directory) @@ -549,10 +549,8 @@ get_extension_script_filename(ExtensionControlFile *control, * fields of *control. We parse primary file if version == NULL, * else the optional auxiliary file for that version. * - * The control file will be search on Extension_control_path paths if - * control->control_dir is NULL, otherwise it will use the value of control_dir - * to read and parse the .control file, so it assume that the control_dir is a - * valid path for the control file being parsed. + * If control->control_dir is not NULL, use that to read and parse the + * control file, otherwise search for the file in extension_control_path. * * Control files are supposed to be very short, half a dozen lines, * so we don't worry about memory allocation risks here. Also we don't @@ -2300,7 +2298,7 @@ pg_available_extensions(PG_FUNCTION_ARGS) /* * Ignore already-found names. They are not reachable by the - * path search, so don't shown them. + * path search, so don't show them. */ extname_str = makeString(extname); if (list_member(found_ext, extname_str)) @@ -3949,12 +3947,10 @@ new_ExtensionControlFile(const char *extname) } /* - * Work in a very similar way with find_in_path but it receives an already - * parsed List of paths to search the basename and it do not support macro - * replacement or custom error messages (for simplicity). + * Search for the basename in the list of paths. * - * By "already parsed List of paths" this function expected that paths already - * have all macros replaced. + * Similar to find_in_path but for simplicity does not support custom error + * messages and expects that paths already have all macros replaced. */ char * find_in_paths(const char *basename, List *paths) -- 2.52.0
