Re: [PATCH 3/3 v2] Load system gdbinit files from a directory

["Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>]

Thanks for the review!

On Mon, Aug 26, 2019 at 2:22 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > Date: Sun, 25 Aug 2019 19:33:10 -0500
> > From: "Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>
> > Cc: Christian Biesinger <cbiesinger@google.com>
> >
> > Adds a configure option --with-system-gdbinit-dir to specify a directory
> > in which to look for gdbinit files. All files in this directory are
> > loaded on startup (subject to -n/-nx as usual).
>
> I think this option's value should be displayed by "show
> configuration" and by "gdb --config".

Done.

> > +* If configured with --with-system-gdbinit-dir, GDB will now also load
>
> I think "also" here might be confusing, because it lacks context.  I
> think we should say explicitly "in addition to the system-wide gdbinit
> file" instead.

Done.

> > +  all files in that directory as system gdbinit files, unless the -nx
> > +  or -n flag is provided. These files can be written in any supported
>
> Two spaces between sentences (here and elsewhere throughout the
> patch).

I think I fixed all occurrences. (searching gdb.texinfo for "\. [^ ]"
found a lot of existing places that do it wrong :( )

> > +@item @file{system.gdbinit.d}
> > +This is the system-wide init directory.
> > +Its location is specified with the @code{--with-system-gdbinit-dir}
> > +configure option (@pxref{System-wide configuration}).
> > +Files in this directory are loaded immediately after system.gdbinit (if
> > +enabled) when @value{GDBN} starts, before command line options have been
> > +processed.
>
> I'm not sure I understand: _all_ files in that directory will be
> loaded, regardless of how they are named?  If so, I think we should
> say that explicitly.

I changed it now so that only .gdb/.py/.scm is loaded, and documented that.

> We should probably also say that the order the
> files are loaded is arbitrary.

They're actually alphabetically sorted; added a note in the
documentation about that.

>  Also, we should say something about
> that directory including subdirectories, because I think the reader
> might wonder about that.

Done.

> >  @cindex init file
> >  Reads the system-wide @dfn{init file} (if @option{--with-system-gdbinit} was
> >  used when building @value{GDBN}; @pxref{System-wide configuration,
> > - ,System-wide configuration and settings}) and executes all the commands in
> > -that file.
> > + ,System-wide configuration and settings}) and the files in the system-wide
> > +gdbinit directory (if @option{--with-system-gdbinit-dir} was used) and executes
> > +all the commands in those files. If scripting languages are enabled, the files
> > +can be written in any of the supported languages as long as the extension matches
> > +the language.
>
> Is the order of reading as described, i.e. the file first, then the
> files in the directory?

Yes.

> Btw, how does this option interact with auto-load safe-path?  Would
> GDB refuse to load init files from this directory if it considers them
> "unsafe"?

No, that is not consulted for these files (if it were, the system
gdbinit file or ~/.gdbinit would not work in the common case, because
the auto-load path is $debugdir:$datadir/auto-load)

> > +@item --with-system-gdbinit-dir=@var{directory}
> > +Configure @value{GDBN} to automatically load init files from a
> > +system-wide directory.  @var{directory} should be an absolute path.
>                                                         ^^^^^^^^^^^^^
> "absolute file name".  The Gnu Coding Standards frown on using "path"
> for anything that is not PATH-style lists of directories.

Changed to "absolute directory name"; I didn't want to use "file name"
for a directory.

(looks like a number of other occurrences in gdb.texinfo should be
changed too...)

> > +@ifset SYSTEM_GDBINIT
> > +@value{SYSTEM_GDBINIT_DIR}/*
> > +@end ifset
>
> @ifset SYSTEM_GDBINIT or @ifset SYSTEM_GDBINIT_DIR?

Thanks, fixed.

Will send a new patch in a second.

Christian