[4/6] gdb: add match formatter mechanism for 'complete' command output

Message ID 6adc14efeac88ecd9501c0c8c53b622099333792.1711712401.git.aburgess@redhat.com
State New
Headers
Series Further filename completion improvements |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Andrew Burgess March 29, 2024, 11:42 a.m. UTC
  This commit solves the problem that was made worse in the previous
commit.  When completing a filename with the 'complete' command GDB
will always add a trailing quote character, even if the completion is
a directory name, in which case it would be better if the trailing
quote was not added.  Consider:

  (gdb) complete file '/tmp/xx
  file '/tmp/xxx/'

The completion offered here is really only a partial completion, we've
completed up to the end of the next directory name, but, until we have
a filename then the completion is not finished and the trailing quote
should not be added.

This would match the readline behaviour, e.g.:

  (gdb) file '/tmp/xx<TAB>
  (gdb) file '/tmp/xxx/

In this case readline completes the directory name, but doesn't add
the trailing quote character.

To achieve this, in this commit, I've added a new function pointer
member variable completion_result::m_match_formatter.  This contains a
pointer to a callback function which is used by the 'complete' command
to format each completion result.

The default behaviour of this callback function is to just append the
quote character (the character from before the completion string) to
the end of the completion result.

However, for filename completion we override the default value of
m_match_formatter, this new function checks if the completion result
is a directory or not.  If the completion result is a directory then
the closing quote is not added, instead we add a trailing '/'
character.

The code to add a trailing '/' character already exists within the
filename_completer function.  This is no longer needed in this
location, instead this code is move into the formatter callback.

Tests are updated to handle the changes in functionality.
---
 gdb/completer.c                               | 92 ++++++++++++++-----
 gdb/completer.h                               | 42 ++++++++-
 .../gdb.base/filename-completion.exp          | 66 +++++++++----
 3 files changed, 155 insertions(+), 45 deletions(-)
  

Comments

Lancelot SIX March 30, 2024, 11:49 p.m. UTC | #1
Hi Andrew,

A couple of comments below.

On Fri, Mar 29, 2024 at 11:42:30AM +0000, Andrew Burgess wrote:
> This commit solves the problem that was made worse in the previous
> commit.  When completing a filename with the 'complete' command GDB
> will always add a trailing quote character, even if the completion is
> a directory name, in which case it would be better if the trailing
> quote was not added.  Consider:
> 
>   (gdb) complete file '/tmp/xx
>   file '/tmp/xxx/'
> 
> The completion offered here is really only a partial completion, we've
> completed up to the end of the next directory name, but, until we have
> a filename then the completion is not finished and the trailing quote
> should not be added.
> 
> This would match the readline behaviour, e.g.:
> 
>   (gdb) file '/tmp/xx<TAB>
>   (gdb) file '/tmp/xxx/
> 
> In this case readline completes the directory name, but doesn't add
> the trailing quote character.
> 
> To achieve this, in this commit, I've added a new function pointer
> member variable completion_result::m_match_formatter.  This contains a
> pointer to a callback function which is used by the 'complete' command
> to format each completion result.
> 
> The default behaviour of this callback function is to just append the
> quote character (the character from before the completion string) to
> the end of the completion result.
> 
> However, for filename completion we override the default value of
> m_match_formatter, this new function checks if the completion result
> is a directory or not.  If the completion result is a directory then
> the closing quote is not added, instead we add a trailing '/'
> character.
> 
> The code to add a trailing '/' character already exists within the
> filename_completer function.  This is no longer needed in this
> location, instead this code is move into the formatter callback.
> 
> Tests are updated to handle the changes in functionality.
> ---
>  gdb/completer.c                               | 92 ++++++++++++++-----
>  gdb/completer.h                               | 42 ++++++++-
>  .../gdb.base/filename-completion.exp          | 66 +++++++++----
>  3 files changed, 155 insertions(+), 45 deletions(-)
> 
> diff --git a/gdb/completer.c b/gdb/completer.c
> index 2b3972213d8..785fb09b4d7 100644
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> @@ -351,6 +351,34 @@ gdb_completer_file_name_quote (char *text, int match_type, char *quote_ptr)
>    return strdup (str.c_str ());
>  }
>  
> +/* The function is used to update the completion word MATCH before
> +   displaying it to the user in the 'complete' command output, this
> +   particular function is only used when MATCH has been supplied by the
> +   filename_completer function (and so MATCH is a filename or directory
> +   name).
> +
> +   This function checks to see if the completion word MATCH is a directory,
> +   in which case a trailing "/" (forward-slash) is added, otherwise
> +   QUOTE_CHAR is added as a trailing quote.
> +
> +   Return the updated completion word as a string.  */
> +
> +static std::string
> +filename_match_formatter (const char *match, char quote_char)
> +{
> +  std::string expanded = gdb_tilde_expand (match);
> +  struct stat finfo;
> +  const bool isdir = (stat (expanded.c_str (), &finfo) == 0
> +		      && S_ISDIR (finfo.st_mode));

This could be "std::filesystem::is_derictory (expanded)".  Not sure if
there is a strong benefit though.

> +  std::string result (match);
> +  if (isdir)
> +    result += "/";
> +  else
> +    result += quote_char;
> +
> +  return result;
> +}
> +
>  /* Complete on filenames.  */
>  
>  void
> @@ -361,6 +389,8 @@ filename_completer (struct cmd_list_element *ignore,
>    rl_char_is_quoted_p = gdb_completer_file_name_char_is_quoted;
>    rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
>  
> +  tracker.set_match_format_func (filename_match_formatter);
> +
>    int subsequent_name = 0;
>    while (1)
>      {
> @@ -379,20 +409,6 @@ filename_completer (struct cmd_list_element *ignore,
>        if (p[strlen (p) - 1] == '~')
>  	continue;
>  
> -      /* Readline appends a trailing '/' if the completion is a
> -	 directory.  If this completion request originated from outside
> -	 readline (e.g. GDB's 'complete' command), then we append the
> -	 trailing '/' ourselves now.  */
> -      if (!tracker.from_readline ())
> -	{
> -	  std::string expanded = gdb_tilde_expand (p_rl.get ());
> -	  struct stat finfo;
> -	  const bool isdir = (stat (expanded.c_str (), &finfo) == 0
> -			      && S_ISDIR (finfo.st_mode));
> -	  if (isdir)
> -	    p_rl.reset (concat (p_rl.get (), "/", nullptr));
> -	}
> -
>        tracker.add_completion
>  	(make_completion_match_str (std::move (p_rl), word, word));
>      }
> @@ -1630,10 +1646,25 @@ int max_completions = 200;
>  /* Initial size of the table.  It automagically grows from here.  */
>  #define INITIAL_COMPLETION_HTAB_SIZE 200
>  
> +/* The function is used to update the completion word MATCH before
> +   displaying it to the user in the 'complete' command output.  This
> +   default function is used in all cases except those where a completion
> +   function overrides this function by calling set_match_format_func.
> +
> +   This function returns MATCH with QUOTE_CHAR appended.  If QUOTE_CHAR is
> +   the null-character then the returned string will just contain MATCH.  */
> +
> +static std::string
> +default_match_formatter (const char *match, char quote_char)
> +{
> +  return std::string (match) + quote_char;
> +}
> +
>  /* See completer.h.  */
>  
>  completion_tracker::completion_tracker (bool from_readline)
> -  : m_from_readline (from_readline)
> +  : m_from_readline (from_readline),
> +    m_match_format_func (default_match_formatter)
>  {
>    discard_completions ();
>  }
> @@ -2336,7 +2367,8 @@ completion_tracker::build_completion_result (const char *text,
>  
>        match_list[1] = nullptr;
>  
> -      return completion_result (match_list, 1, completion_suppress_append);
> +      return completion_result (match_list, 1, completion_suppress_append,
> +				m_match_format_func);
>      }
>    else
>      {
> @@ -2373,7 +2405,8 @@ completion_tracker::build_completion_result (const char *text,
>        htab_traverse_noresize (m_entries_hash.get (), func, &builder);
>        match_list[builder.index] = NULL;
>  
> -      return completion_result (match_list, builder.index - 1, false);
> +      return completion_result (match_list, builder.index - 1, false,
> +				m_match_format_func);
>      }
>  }
>  
> @@ -2381,17 +2414,20 @@ completion_tracker::build_completion_result (const char *text,
>  
>  completion_result::completion_result ()
>    : match_list (NULL), number_matches (0),
> -    completion_suppress_append (false)
> +    completion_suppress_append (false),
> +    m_match_formatter (nullptr)

Is there a reason not to initialize this with default_match_formatter?

Ensuring that we can not assign nullptr to the m_match_formatter field
could avoid the need of checking that this field is not nullptr before
using it, and more importantly it makes it easier to track down an issue
if someone tries to set it to nullptr at some point.

We could also define match_fonmat_func_t to be

    using match_format_func_t
      = std::function<std::stirng (const char *match, char quote_char)>;

and have

    match_format_func_t m_match_formatter;

In which case I do not think we can have a non-valid callable stored in
_match_formatter.  That being said, std::function is not a light
abstraction (as far a I know), so I can understand if you would prefer
not to go that route.

>  {}
>  
>  /* See completer.h  */
>  
>  completion_result::completion_result (char **match_list_,
>  				      size_t number_matches_,
> -				      bool completion_suppress_append_)
> +				      bool completion_suppress_append_,
> +				      match_format_func_t match_formatter_)
>    : match_list (match_list_),
>      number_matches (number_matches_),
> -    completion_suppress_append (completion_suppress_append_)
> +    completion_suppress_append (completion_suppress_append_),
> +    m_match_formatter (match_formatter_)
>  {}
>  
>  /* See completer.h  */
> @@ -2405,10 +2441,12 @@ completion_result::~completion_result ()
>  
>  completion_result::completion_result (completion_result &&rhs) noexcept
>    : match_list (rhs.match_list),
> -    number_matches (rhs.number_matches)
> +    number_matches (rhs.number_matches),
> +    m_match_formatter (rhs.m_match_formatter)
>  {
>    rhs.match_list = NULL;
>    rhs.number_matches = 0;
> +  rhs.m_match_formatter = nullptr;

Similarly, we might want to reset to a legal value here (i.e.
default_match_formatter).

>  }
>  
>  /* See completer.h  */
> @@ -2458,12 +2496,18 @@ completion_result::print_matches (const std::string &prefix,
>  {
>    this->sort_match_list ();
>  
> -  char buf[2] = { (char) quote_char, '\0' };
>    size_t off = this->number_matches == 1 ? 0 : 1;
>  
>    for (size_t i = 0; i < this->number_matches; i++)
> -    printf_unfiltered ("%s%s%s\n", prefix.c_str (),
> -		       this->match_list[i + off], buf);
> +    {
> +      gdb_assert (this->m_match_formatter != nullptr);
> +      std::string formatted_match
> +	= this->m_match_formatter (this->match_list[i + off],
> +				   (char) quote_char);
> +
> +      printf_unfiltered ("%s%s\n", prefix.c_str (),
> +			 formatted_match.c_str ());
> +    }
>  
>    if (this->number_matches == max_completions)
>      {
> diff --git a/gdb/completer.h b/gdb/completer.h
> index 4419c8f6d30..8965ecacc44 100644
> --- a/gdb/completer.h
> +++ b/gdb/completer.h
> @@ -247,12 +247,24 @@ struct completion_match_result
>  
>  struct completion_result
>  {
> +  /* The type of a function that is used to format completion results when
> +     using the 'complete' command.  MATCH is the completion word to be
> +     printed, and QUOTE_CHAR is a trailing quote character to (possibly)
> +     add at the end of MATCH.  QUOTE_CHAR can be the null-character in
> +     which case no trailing quote should be added.
> +
> +     Return the possibly modified completion match word which should be
> +     presented to the user.  */
> +  using match_format_func_t = std::string (*) (const char *match,
> +					       char quote_char);
> +
>    /* Create an empty result.  */
>    completion_result ();
>  
>    /* Create a result.  */
>    completion_result (char **match_list, size_t number_matches,
> -		     bool completion_suppress_append);
> +		     bool completion_suppress_append,
> +		     match_format_func_t match_format_func);
>  
>    /* Destroy a result.  */
>    ~completion_result ();
> @@ -274,10 +286,15 @@ struct completion_result
>       completions, otherwise, each line of output consists of PREFIX
>       followed by one of the possible completion words.
>  
> -     The QUOTE_CHAR is appended after each possible completion word and
> -     should be the quote character that appears before the completion word,
> -     or the null-character if there is no quote before the completion
> -     word.  */
> +     The QUOTE_CHAR is usually appended after each possible completion
> +     word and should be the quote character that appears before the
> +     completion word, or the null-character if there is no quote before
> +     the completion word.
> +
> +     The QUOTE_CHAR is not always appended to the completion output.  For
> +     example, filename completions will not append QUOTE_CHAR if the
> +     completion is a directory name.  This is all handled by calling this
> +     function.  */
>    void print_matches (const std::string &prefix, const char *word,
>  		      int quote_char);
>  
> @@ -305,6 +322,12 @@ struct completion_result
>    /* Whether readline should suppress appending a whitespace, when
>       there's only one possible completion.  */
>    bool completion_suppress_append;
> +
> +private:
> +  /* A function which formats a single completion match ready for display
> +     as part of the 'complete' command output.  Different completion
> +     functions can set different formatter functions.  */
> +  match_format_func_t m_match_formatter;
>  };
>  
>  /* Object used by completers to build a completion match list to hand
> @@ -441,6 +464,11 @@ class completion_tracker
>    bool from_readline () const
>    { return m_from_readline; }
>  
> +  /* Set the function used to format the completion word before displaying
> +     it to the user to F, this is used by the 'complete' command.  */
> +  void set_match_format_func (completion_result::match_format_func_t f)
> +  { m_match_format_func = f; }

If you prefer to keep a plain function pointer, I’d have an assertion
here to ensure the field is not set to nullptr (just to make it easier
to track down invalid usage).

Best,
Lancelot.

> +
>  private:
>  
>    /* The type that we place into the m_entries_hash hash table.  */
> @@ -535,6 +563,10 @@ class completion_tracker
>       interactively. The 'complete' command is a way to generate completions
>       not to be displayed by readline.  */
>    bool m_from_readline;
> +
> +  /* The function used to format the completion word before it is printed
> +     in the 'complete' command output.  */
> +  completion_result::match_format_func_t m_match_format_func;
>  };
>  
>  /* Return a string to hand off to readline as a completion match
> diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
> index 66e5f411795..d7c99e1340d 100644
> --- a/gdb/testsuite/gdb.base/filename-completion.exp
> +++ b/gdb/testsuite/gdb.base/filename-completion.exp
> @@ -46,6 +46,49 @@ proc setup_directory_tree {} {
>      return $root
>  }
>  
> +# This proc started as a copy of test_gdb_complete_multiple, however, this
> +# version does some extra work.  See the original test_gdb_complete_multiple
> +# for a description of all the arguments.
> +#
> +# When using the 'complete' command with filenames, GDB will add a trailing
> +# quote for filenames, and a trailing "/" for directory names.  As the
> +# trailing "/" is also added in the tab-completion output the
> +# COMPLETION_LIST will include the "/" character, but the trailing quote is
> +# only added when using the 'complete' command.
> +#
> +# Pass the trailing quote will be passed as END_QUOTE_CHAR, this proc will
> +# run the tab completion test, and will then add the trailing quote to those
> +# entries in COMPLETION_LIST that don't have a trailing "/" before running
> +# the 'complete' command test.
> +proc test_gdb_complete_filename_multiple {
> +  cmd_prefix completion_word add_completed_line completion_list
> +  {start_quote_char ""} {end_quote_char ""} {max_completions false}
> +  {testname ""}
> +} {
> +    if { [readline_is_used] } {
> +	test_gdb_complete_tab_multiple "$cmd_prefix$completion_word" \
> +	    $add_completed_line $completion_list $max_completions $testname
> +    }
> +
> +    if { $start_quote_char eq "" && $end_quote_char ne "" } {
> +	set updated_completion_list {}
> +
> +	foreach entry $completion_list {
> +	    if {[string range $entry end end] ne "/"} {
> +		set entry $entry$end_quote_char
> +	    }
> +	    lappend updated_completion_list $entry
> +	}
> +
> +	set completion_list $updated_completion_list
> +	set end_quote_char ""
> +    }
> +
> +    test_gdb_complete_cmd_multiple $cmd_prefix $completion_word \
> +	$completion_list $start_quote_char $end_quote_char $max_completions \
> +	$testname
> +}
> +
>  # Run filename completetion tests.  ROOT is the base directory as
>  # returned from setup_directory_tree, though, if ROOT is a
>  # sub-directory of the user's home directory ROOT might have been
> @@ -63,31 +106,22 @@ proc run_tests { root } {
>  	test_gdb_complete_none "file ${qc}${root}/xx" \
>  	    "expand a non-existent filename"
>  
> -	# The following test is split into separate cmd and tab calls as the
> -	# cmd versions will add a closing quote.  It shouldn't be doing
> -	# this; this will be fixed in a later commit.
> -	test_gdb_complete_cmd_unique "file ${qc}${root}/a" \
> -	    "file ${qc}${root}/aaa/${qc}" \
> +	test_gdb_complete_unique "file ${qc}${root}/a" \
> +	    "file ${qc}${root}/aaa/" "" false \
>  	    "expand a unique directory name"
>  
> -	if { [readline_is_used] } {
> -	    test_gdb_complete_tab_unique "file ${qc}${root}/a" \
> -		"file ${qc}${root}/aaa/" "" \
> -		"expand a unique directory name"
> -	}
> -
>  	test_gdb_complete_unique "file ${qc}${root}/cc2" \
>  	    "file ${qc}${root}/cc2${qc}" " " false \
>  	    "expand a unique filename"
>  
> -	test_gdb_complete_multiple "file ${qc}${root}/" \
> +	test_gdb_complete_filename_multiple "file ${qc}${root}/" \
>  	    "b" "b" {
>  		"bb1/"
>  		"bb2/"
> -	    } "" "${qc}" false \
> +	    } "" "" false \
>  	    "expand multiple directory names"
>  
> -	test_gdb_complete_multiple "file ${qc}${root}/" \
> +	test_gdb_complete_filename_multiple "file ${qc}${root}/" \
>  	    "c" "c" {
>  		"cc1/"
>  		"cc2"
> @@ -99,14 +133,14 @@ proc run_tests { root } {
>  	if { $qc ne "" } {
>  	    set sp " "
>  
> -	    test_gdb_complete_multiple "file ${qc}${root}/aaa/" \
> +	    test_gdb_complete_filename_multiple "file ${qc}${root}/aaa/" \
>  		"a" "a${sp}" {
>  		    "aa bb"
>  		    "aa cc"
>  		} "" "${qc}" false \
>  		"expand filenames containing spaces"
>  
> -	    test_gdb_complete_multiple "file ${qc}${root}/bb1/" \
> +	    test_gdb_complete_filename_multiple "file ${qc}${root}/bb1/" \
>  		"a" "a" {
>  		    "aa\"bb"
>  		    "aa'bb"
> -- 
> 2.25.4
>
  
Andrew Burgess April 12, 2024, 5:42 p.m. UTC | #2
Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sat, 30 Mar 2024 23:49:18 +0000
>> From: Lancelot SIX <lsix@lancelotsix.com>
>> Cc: gdb-patches@sourceware.org
>> 
>> >   (gdb) complete file '/tmp/xx
>> >   file '/tmp/xxx/'
>> > 
>> > The completion offered here is really only a partial completion, we've
>> > completed up to the end of the next directory name, but, until we have
>> > a filename then the completion is not finished and the trailing quote
>> > should not be added.
>> > 
>> > This would match the readline behaviour, e.g.:
>> > 
>> >   (gdb) file '/tmp/xx<TAB>
>> >   (gdb) file '/tmp/xxx/
>> > 
>> > In this case readline completes the directory name, but doesn't add
>> > the trailing quote character.
>
> Btw, what readline does is not the only useful behavior.  An
> alternative would be this:
>
>   (gdb) file '/tmp/xx<TAB>
>    => (gdb) file '/tmp/xx/'
>   (gdb) file '/tmp/xx/'a<TAB>
>    => (gdb) file '/tmp/xx/abcd'
>
> That is, allow to type after the closing quote, and remove the closing
> quote before trying to complete.  This way, the user can use
> '/tmp/xx/' if that is what she needs, or continue typing and
> completing if not.

That does sound better.  I think there are still things that would need
figuring out.  For example, what if the user didn't do the second tab
completion, but just kept typing.

  (gdb) file '/tmp/xx<TAB>
	=> (gdb) file '/tmp/xx/'
  (gdb) file '/tmp/xx/'abcd<ENTER>
	=> ?? What would this do?

What if there really was a file /tmp/xx/'abcd ? What happens then?  But
I do think this sounds like an interesting idea.  But...

... I don't think I want to start making such sweeping changes to
readline, or how GDB uses readline.  Or maybe that would require moving
off readline completely?

My goal with this work is to support '-filename PATH' command options
using GDB's option framework.  Adding such options really calls for good
filename completion.  [ NOTE: I'm not adding those options in this
series, that would be the next project. ]

Ideally I want to work with GDB as it is right now, i.e. how does GDB
work right now, and how does GDB parse filenames right now, and have
filename completion work with those constraints.

Thanks,
Andrew
  
Eli Zaretskii April 12, 2024, 6:44 p.m. UTC | #3
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Fri, 12 Apr 2024 18:42:12 +0100
> 
> >   (gdb) file '/tmp/xx<TAB>
> >    => (gdb) file '/tmp/xx/'
> >   (gdb) file '/tmp/xx/'a<TAB>
> >    => (gdb) file '/tmp/xx/abcd'
> >
> > That is, allow to type after the closing quote, and remove the closing
> > quote before trying to complete.  This way, the user can use
> > '/tmp/xx/' if that is what she needs, or continue typing and
> > completing if not.
> 
> That does sound better.  I think there are still things that would need
> figuring out.  For example, what if the user didn't do the second tab
> completion, but just kept typing.
> 
>   (gdb) file '/tmp/xx<TAB>
> 	=> (gdb) file '/tmp/xx/'
>   (gdb) file '/tmp/xx/'abcd<ENTER>
> 	=> ?? What would this do?
> 
> What if there really was a file /tmp/xx/'abcd ? What happens then?

ENTER does the same as TAB, and then submits the result.  So GDB will
see '/tmp/xx/abcd' in that case.
  
Andrew Burgess April 12, 2024, 10:29 p.m. UTC | #4
Eli Zaretskii <eliz@gnu.org> writes:

>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: gdb-patches@sourceware.org
>> Date: Fri, 12 Apr 2024 18:42:12 +0100
>> 
>> >   (gdb) file '/tmp/xx<TAB>
>> >    => (gdb) file '/tmp/xx/'
>> >   (gdb) file '/tmp/xx/'a<TAB>
>> >    => (gdb) file '/tmp/xx/abcd'
>> >
>> > That is, allow to type after the closing quote, and remove the closing
>> > quote before trying to complete.  This way, the user can use
>> > '/tmp/xx/' if that is what she needs, or continue typing and
>> > completing if not.
>> 
>> That does sound better.  I think there are still things that would need
>> figuring out.  For example, what if the user didn't do the second tab
>> completion, but just kept typing.
>> 
>>   (gdb) file '/tmp/xx<TAB>
>> 	=> (gdb) file '/tmp/xx/'
>>   (gdb) file '/tmp/xx/'abcd<ENTER>
>> 	=> ?? What would this do?
>> 
>> What if there really was a file /tmp/xx/'abcd ? What happens then?
>
> ENTER does the same as TAB, and then submits the result.  So GDB will
> see '/tmp/xx/abcd' in that case.

This all sounds great.  Do you know of any other readline based projects
that do completion like this?

My concern here is that, as I understand readline, this isn't going to
be easy to implement.  And even if we could (somehow) convince readline
to do this, it's going to result in a bunch of extra complexity compared
to just having readline do things the way it wants to.

And no, GDB doesn't have to use readline, but I'd rather not have to
throw out readline just so we can have working filename completion.

I'm not trying to be dismissive here.  This started as "Andrew's come up
with a design, he should have discussed it first."  And I'm trying to
work out if this is a serious, we should do it this other way.  Or if
this is just "See, there are other ways it could be done, you should
have discussed it first"?

To both of these my answer would be, I've not really invented anything
here, I've just connected up GDB with readline, and what we get is the
readline way of doing things.

Maybe then you know that readline can be made to do things differently
(hence me asking for examples).  Or maybe you're suggesting we should
forget readline and do things as you suggest ... I'm just trying to
figure out what you ideal end result is here.

Thanks,
Andrew
  
Eli Zaretskii April 13, 2024, 6:39 a.m. UTC | #5
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: lsix@lancelotsix.com, gdb-patches@sourceware.org
> Date: Fri, 12 Apr 2024 23:29:02 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >>   (gdb) file '/tmp/xx<TAB>
> >> 	=> (gdb) file '/tmp/xx/'
> >>   (gdb) file '/tmp/xx/'abcd<ENTER>
> >> 	=> ?? What would this do?
> >> 
> >> What if there really was a file /tmp/xx/'abcd ? What happens then?
> >
> > ENTER does the same as TAB, and then submits the result.  So GDB will
> > see '/tmp/xx/abcd' in that case.
> 
> This all sounds great.  Do you know of any other readline based projects
> that do completion like this?

No, not off the top of my head.

Ironically, the Windows shell cmd.exe does behave like that.

> Maybe then you know that readline can be made to do things differently
> (hence me asking for examples).  Or maybe you're suggesting we should
> forget readline and do things as you suggest ... I'm just trying to
> figure out what you ideal end result is here.

If Readline doesn't want to go this way, I think in the long run GDB
should develop its own completion machinery.  But I'm okay with
leaving that alone for now.
  

Patch

diff --git a/gdb/completer.c b/gdb/completer.c
index 2b3972213d8..785fb09b4d7 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -351,6 +351,34 @@  gdb_completer_file_name_quote (char *text, int match_type, char *quote_ptr)
   return strdup (str.c_str ());
 }
 
+/* The function is used to update the completion word MATCH before
+   displaying it to the user in the 'complete' command output, this
+   particular function is only used when MATCH has been supplied by the
+   filename_completer function (and so MATCH is a filename or directory
+   name).
+
+   This function checks to see if the completion word MATCH is a directory,
+   in which case a trailing "/" (forward-slash) is added, otherwise
+   QUOTE_CHAR is added as a trailing quote.
+
+   Return the updated completion word as a string.  */
+
+static std::string
+filename_match_formatter (const char *match, char quote_char)
+{
+  std::string expanded = gdb_tilde_expand (match);
+  struct stat finfo;
+  const bool isdir = (stat (expanded.c_str (), &finfo) == 0
+		      && S_ISDIR (finfo.st_mode));
+  std::string result (match);
+  if (isdir)
+    result += "/";
+  else
+    result += quote_char;
+
+  return result;
+}
+
 /* Complete on filenames.  */
 
 void
@@ -361,6 +389,8 @@  filename_completer (struct cmd_list_element *ignore,
   rl_char_is_quoted_p = gdb_completer_file_name_char_is_quoted;
   rl_completer_quote_characters = gdb_completer_file_name_quote_characters;
 
+  tracker.set_match_format_func (filename_match_formatter);
+
   int subsequent_name = 0;
   while (1)
     {
@@ -379,20 +409,6 @@  filename_completer (struct cmd_list_element *ignore,
       if (p[strlen (p) - 1] == '~')
 	continue;
 
-      /* Readline appends a trailing '/' if the completion is a
-	 directory.  If this completion request originated from outside
-	 readline (e.g. GDB's 'complete' command), then we append the
-	 trailing '/' ourselves now.  */
-      if (!tracker.from_readline ())
-	{
-	  std::string expanded = gdb_tilde_expand (p_rl.get ());
-	  struct stat finfo;
-	  const bool isdir = (stat (expanded.c_str (), &finfo) == 0
-			      && S_ISDIR (finfo.st_mode));
-	  if (isdir)
-	    p_rl.reset (concat (p_rl.get (), "/", nullptr));
-	}
-
       tracker.add_completion
 	(make_completion_match_str (std::move (p_rl), word, word));
     }
@@ -1630,10 +1646,25 @@  int max_completions = 200;
 /* Initial size of the table.  It automagically grows from here.  */
 #define INITIAL_COMPLETION_HTAB_SIZE 200
 
+/* The function is used to update the completion word MATCH before
+   displaying it to the user in the 'complete' command output.  This
+   default function is used in all cases except those where a completion
+   function overrides this function by calling set_match_format_func.
+
+   This function returns MATCH with QUOTE_CHAR appended.  If QUOTE_CHAR is
+   the null-character then the returned string will just contain MATCH.  */
+
+static std::string
+default_match_formatter (const char *match, char quote_char)
+{
+  return std::string (match) + quote_char;
+}
+
 /* See completer.h.  */
 
 completion_tracker::completion_tracker (bool from_readline)
-  : m_from_readline (from_readline)
+  : m_from_readline (from_readline),
+    m_match_format_func (default_match_formatter)
 {
   discard_completions ();
 }
@@ -2336,7 +2367,8 @@  completion_tracker::build_completion_result (const char *text,
 
       match_list[1] = nullptr;
 
-      return completion_result (match_list, 1, completion_suppress_append);
+      return completion_result (match_list, 1, completion_suppress_append,
+				m_match_format_func);
     }
   else
     {
@@ -2373,7 +2405,8 @@  completion_tracker::build_completion_result (const char *text,
       htab_traverse_noresize (m_entries_hash.get (), func, &builder);
       match_list[builder.index] = NULL;
 
-      return completion_result (match_list, builder.index - 1, false);
+      return completion_result (match_list, builder.index - 1, false,
+				m_match_format_func);
     }
 }
 
@@ -2381,17 +2414,20 @@  completion_tracker::build_completion_result (const char *text,
 
 completion_result::completion_result ()
   : match_list (NULL), number_matches (0),
-    completion_suppress_append (false)
+    completion_suppress_append (false),
+    m_match_formatter (nullptr)
 {}
 
 /* See completer.h  */
 
 completion_result::completion_result (char **match_list_,
 				      size_t number_matches_,
-				      bool completion_suppress_append_)
+				      bool completion_suppress_append_,
+				      match_format_func_t match_formatter_)
   : match_list (match_list_),
     number_matches (number_matches_),
-    completion_suppress_append (completion_suppress_append_)
+    completion_suppress_append (completion_suppress_append_),
+    m_match_formatter (match_formatter_)
 {}
 
 /* See completer.h  */
@@ -2405,10 +2441,12 @@  completion_result::~completion_result ()
 
 completion_result::completion_result (completion_result &&rhs) noexcept
   : match_list (rhs.match_list),
-    number_matches (rhs.number_matches)
+    number_matches (rhs.number_matches),
+    m_match_formatter (rhs.m_match_formatter)
 {
   rhs.match_list = NULL;
   rhs.number_matches = 0;
+  rhs.m_match_formatter = nullptr;
 }
 
 /* See completer.h  */
@@ -2458,12 +2496,18 @@  completion_result::print_matches (const std::string &prefix,
 {
   this->sort_match_list ();
 
-  char buf[2] = { (char) quote_char, '\0' };
   size_t off = this->number_matches == 1 ? 0 : 1;
 
   for (size_t i = 0; i < this->number_matches; i++)
-    printf_unfiltered ("%s%s%s\n", prefix.c_str (),
-		       this->match_list[i + off], buf);
+    {
+      gdb_assert (this->m_match_formatter != nullptr);
+      std::string formatted_match
+	= this->m_match_formatter (this->match_list[i + off],
+				   (char) quote_char);
+
+      printf_unfiltered ("%s%s\n", prefix.c_str (),
+			 formatted_match.c_str ());
+    }
 
   if (this->number_matches == max_completions)
     {
diff --git a/gdb/completer.h b/gdb/completer.h
index 4419c8f6d30..8965ecacc44 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -247,12 +247,24 @@  struct completion_match_result
 
 struct completion_result
 {
+  /* The type of a function that is used to format completion results when
+     using the 'complete' command.  MATCH is the completion word to be
+     printed, and QUOTE_CHAR is a trailing quote character to (possibly)
+     add at the end of MATCH.  QUOTE_CHAR can be the null-character in
+     which case no trailing quote should be added.
+
+     Return the possibly modified completion match word which should be
+     presented to the user.  */
+  using match_format_func_t = std::string (*) (const char *match,
+					       char quote_char);
+
   /* Create an empty result.  */
   completion_result ();
 
   /* Create a result.  */
   completion_result (char **match_list, size_t number_matches,
-		     bool completion_suppress_append);
+		     bool completion_suppress_append,
+		     match_format_func_t match_format_func);
 
   /* Destroy a result.  */
   ~completion_result ();
@@ -274,10 +286,15 @@  struct completion_result
      completions, otherwise, each line of output consists of PREFIX
      followed by one of the possible completion words.
 
-     The QUOTE_CHAR is appended after each possible completion word and
-     should be the quote character that appears before the completion word,
-     or the null-character if there is no quote before the completion
-     word.  */
+     The QUOTE_CHAR is usually appended after each possible completion
+     word and should be the quote character that appears before the
+     completion word, or the null-character if there is no quote before
+     the completion word.
+
+     The QUOTE_CHAR is not always appended to the completion output.  For
+     example, filename completions will not append QUOTE_CHAR if the
+     completion is a directory name.  This is all handled by calling this
+     function.  */
   void print_matches (const std::string &prefix, const char *word,
 		      int quote_char);
 
@@ -305,6 +322,12 @@  struct completion_result
   /* Whether readline should suppress appending a whitespace, when
      there's only one possible completion.  */
   bool completion_suppress_append;
+
+private:
+  /* A function which formats a single completion match ready for display
+     as part of the 'complete' command output.  Different completion
+     functions can set different formatter functions.  */
+  match_format_func_t m_match_formatter;
 };
 
 /* Object used by completers to build a completion match list to hand
@@ -441,6 +464,11 @@  class completion_tracker
   bool from_readline () const
   { return m_from_readline; }
 
+  /* Set the function used to format the completion word before displaying
+     it to the user to F, this is used by the 'complete' command.  */
+  void set_match_format_func (completion_result::match_format_func_t f)
+  { m_match_format_func = f; }
+
 private:
 
   /* The type that we place into the m_entries_hash hash table.  */
@@ -535,6 +563,10 @@  class completion_tracker
      interactively. The 'complete' command is a way to generate completions
      not to be displayed by readline.  */
   bool m_from_readline;
+
+  /* The function used to format the completion word before it is printed
+     in the 'complete' command output.  */
+  completion_result::match_format_func_t m_match_format_func;
 };
 
 /* Return a string to hand off to readline as a completion match
diff --git a/gdb/testsuite/gdb.base/filename-completion.exp b/gdb/testsuite/gdb.base/filename-completion.exp
index 66e5f411795..d7c99e1340d 100644
--- a/gdb/testsuite/gdb.base/filename-completion.exp
+++ b/gdb/testsuite/gdb.base/filename-completion.exp
@@ -46,6 +46,49 @@  proc setup_directory_tree {} {
     return $root
 }
 
+# This proc started as a copy of test_gdb_complete_multiple, however, this
+# version does some extra work.  See the original test_gdb_complete_multiple
+# for a description of all the arguments.
+#
+# When using the 'complete' command with filenames, GDB will add a trailing
+# quote for filenames, and a trailing "/" for directory names.  As the
+# trailing "/" is also added in the tab-completion output the
+# COMPLETION_LIST will include the "/" character, but the trailing quote is
+# only added when using the 'complete' command.
+#
+# Pass the trailing quote will be passed as END_QUOTE_CHAR, this proc will
+# run the tab completion test, and will then add the trailing quote to those
+# entries in COMPLETION_LIST that don't have a trailing "/" before running
+# the 'complete' command test.
+proc test_gdb_complete_filename_multiple {
+  cmd_prefix completion_word add_completed_line completion_list
+  {start_quote_char ""} {end_quote_char ""} {max_completions false}
+  {testname ""}
+} {
+    if { [readline_is_used] } {
+	test_gdb_complete_tab_multiple "$cmd_prefix$completion_word" \
+	    $add_completed_line $completion_list $max_completions $testname
+    }
+
+    if { $start_quote_char eq "" && $end_quote_char ne "" } {
+	set updated_completion_list {}
+
+	foreach entry $completion_list {
+	    if {[string range $entry end end] ne "/"} {
+		set entry $entry$end_quote_char
+	    }
+	    lappend updated_completion_list $entry
+	}
+
+	set completion_list $updated_completion_list
+	set end_quote_char ""
+    }
+
+    test_gdb_complete_cmd_multiple $cmd_prefix $completion_word \
+	$completion_list $start_quote_char $end_quote_char $max_completions \
+	$testname
+}
+
 # Run filename completetion tests.  ROOT is the base directory as
 # returned from setup_directory_tree, though, if ROOT is a
 # sub-directory of the user's home directory ROOT might have been
@@ -63,31 +106,22 @@  proc run_tests { root } {
 	test_gdb_complete_none "file ${qc}${root}/xx" \
 	    "expand a non-existent filename"
 
-	# The following test is split into separate cmd and tab calls as the
-	# cmd versions will add a closing quote.  It shouldn't be doing
-	# this; this will be fixed in a later commit.
-	test_gdb_complete_cmd_unique "file ${qc}${root}/a" \
-	    "file ${qc}${root}/aaa/${qc}" \
+	test_gdb_complete_unique "file ${qc}${root}/a" \
+	    "file ${qc}${root}/aaa/" "" false \
 	    "expand a unique directory name"
 
-	if { [readline_is_used] } {
-	    test_gdb_complete_tab_unique "file ${qc}${root}/a" \
-		"file ${qc}${root}/aaa/" "" \
-		"expand a unique directory name"
-	}
-
 	test_gdb_complete_unique "file ${qc}${root}/cc2" \
 	    "file ${qc}${root}/cc2${qc}" " " false \
 	    "expand a unique filename"
 
-	test_gdb_complete_multiple "file ${qc}${root}/" \
+	test_gdb_complete_filename_multiple "file ${qc}${root}/" \
 	    "b" "b" {
 		"bb1/"
 		"bb2/"
-	    } "" "${qc}" false \
+	    } "" "" false \
 	    "expand multiple directory names"
 
-	test_gdb_complete_multiple "file ${qc}${root}/" \
+	test_gdb_complete_filename_multiple "file ${qc}${root}/" \
 	    "c" "c" {
 		"cc1/"
 		"cc2"
@@ -99,14 +133,14 @@  proc run_tests { root } {
 	if { $qc ne "" } {
 	    set sp " "
 
-	    test_gdb_complete_multiple "file ${qc}${root}/aaa/" \
+	    test_gdb_complete_filename_multiple "file ${qc}${root}/aaa/" \
 		"a" "a${sp}" {
 		    "aa bb"
 		    "aa cc"
 		} "" "${qc}" false \
 		"expand filenames containing spaces"
 
-	    test_gdb_complete_multiple "file ${qc}${root}/bb1/" \
+	    test_gdb_complete_filename_multiple "file ${qc}${root}/bb1/" \
 		"a" "a" {
 		    "aa\"bb"
 		    "aa'bb"