Hi,
On Mon, Oct 24, 2022 at 04:13:51PM +0900, Michael Paquier wrote:
> On Mon, Oct 24, 2022 at 01:33:12PM +0800, Julien Rouhaud wrote:
> > v12 attached, fixing multiple conflicts with recent activity.
>
> typedef struct TokenizedAuthLine
> {
> List *fields; /* List of lists of AuthTokens */
> + char *file_name; /* File name *
>
> Hmm. While putting my eyes on this patch set for the first time in a
> few months (sorry!), the addition of file_name to TokenizedAuthLine in
> 0002 stands out. This impacts all the macros used for the validation
> of the ident and HBA lines, leading as well to a lot of bloat in the
> patch with patterns like that in 20~25 places:
> errcontext("line %d of configuration file \"%s\"", \
> - line_num, IdentFileName))); \
> + line_num, file_name))); \
> [...]
> errcontext("line %d of configuration file \"%s\"",
> - line_num, HbaFileName)));
> + line_num, file_name)));
>
> Do you think that it would make sense to split that into its own
> patch? That would move the code toward less references to HbaFileName
> and IdentFileName, which is one step toward what we want for this
> thread. This tokenization of HBA and ident files is already shared,
> so moving this knowledge into TokenizedAuthLine looks helpful
> independently of the rest.
It would also require to bring HbaLine->sourcefile. I'm afraid it would be
weird to introduce such a refactoring in a separate commit just to pass a
constant down multiple level of indirection, as all the macro will remain
specific to either hba or ident anyway.
I agree that there are quite a lot of s/XXXFileName/file_name/, but those
aren't complicated, and keeping them in the same commit makes it easy to
validate that none has been forgotten since the regression tests covering those
messages are in that commit too.
And of course while double checking that none was forgotten I realize that I
missed the new regcomp_auth_token() which introduced a couple new usage of
HbaFileName.