Re: [PATCH 1/3] gdb::option: Add support for filename option.

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Lancelot SIX <lsix@lancelotsix.com> [2021-02-16 18:52:45 +0000]:

> Le Tue, Feb 16, 2021 at 05:45:01PM +0000, Andrew Burgess a écrit :
> > * Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> [2021-02-13 22:07:50 +0000]:
> > 
> > > This commit adds support for filename option for the gdb::option parsing
> > > framework.  It is meant to help the user enter values that could be
> > > parsed by built_argv.
> > > 
> > > Tab completion works as expected, i.e. similar to bash's behavior.
> > > 
> > > Considering the following tree structure:
> > > 
> > > somedir/
> > > ├── a folder
> > 
> > You probably want a trailing / after 'a folder' to make it clear it is
> > a folder, similar to how somedir/ has the trailing character.
> > 
> > > └── a file
> > > 
> > > The following completions are available:
> > > 
> > >     (gdb) maintenance test-options unknown-is-error -filename somedir/a\ <tab>
> > >     a file    a folder/
> > > 
> > > or
> > > 
> > >     (gdb) maintenance test-options unknown-is-error -filename "somedir/a <tab>
> > >     a file    a folder/
> > > 
> > > The complete command command also provides useful completions which are
> > > escaped or quoted depending on what the user did input initially.
> > > 
> > >     (gdb) complete maintenance test-options unknown-is-error -filename somedir/a
> > >     maintenance test-options unknown-is-error -filename somedir/a\ file
> > >     maintenance test-options unknown-is-error -filename somedir/a\ folder/
> > >     (gdb) complete maintenance test-options unknown-is-error -filename 'somedir/a
> > >     maintenance test-options unknown-is-error -filename 'somedir/a file
> > >     maintenance test-options unknown-is-error -filename 'somedir/a folder/
> > > 
> > > Note that since entering a path is an interactive process, the completer
> > > will not append the closing quote after a directory name. Instead it
> > > adds a '/' and expect the user to provide further input.
> > > 
> > > Having both of those behaviors at the same time requires that the
> > > completer knows if it has been called by readline after the user
> > > pressed the tab key or by some other mechanism.  In the first scenario it
> > > should display non quoted/escaped completions so the user sees actual
> > > filenames but complete quoted/escaped filenames.  In the second scenario
> > > it should only provide quoted/escaped completions.  For that purpose,
> > > this commit adds a m_from_readline property to the completion_tracker
> > > object and initializes it accordingly to the calling context.
> > > 
> > > This filename_completer has slightly different behavior than the one
> > > available in gdb/completer.c mainly with regard to escaping/quoting.
> > > The original completer is kept as is to maintain stable behavior for
> > > commands whose completer might depend on this implementation.  If this is
> > > desirable, I’ll happily replace the existing filename completer with the
> > > one currently proposed in the gdb::options::filename namespace instead
> > > of having both with slightly different behaviors (but I an not sure if
> > > users are relying on the current behavior of the filename
> > > completer).
> > 
> > I wonder if you could expand more on what the differences are, maybe
> > with some examples.  Your new code sounds better, and I don't know why
> > we would want to keep the existing code.
> > 
> The current completer does not handle spaces nor any
> escape mechanism.
> 
> > Ideally I'd like to see some examples of things that the old completer
> > allows us to do that would now break, or behave significantly
> > differently if we just changed over to your code.
> > 
> 
> The only case I see where using the new completer would break things is
> a command which does not use built_argv to parse the argument line:
> 
> static void
> foo_command (const char *args, int from_tty)
> {
>   /* Handle args as a raw filename, or consider that everything that
>      follows a '--' is a filename.  */
>   const char *filename = args;
>   foo(filename);
> }
> 
> Other than that, using a completer that handles space escaping or
> quoting would only improve things.
> 
> I have not tracked down usages of the current completer to make sure no
> command ingests the filename without any sort of processing.  I’ll do that
> before sending a v2 of this patch series.

Based on the above explanation then it sounds like moving to your
completer globally.  It should be fairly easy I'd think to track down
which commands register with the old completer, and from there check
how they process the filename.

If any of them need fixing then I'd do that as a separate patch (in
the same series) first.

Then introduce your new completer and switch everyone over to it.

Thanks,
Andrew