Re: [PATCH] gdb: add trailing '/' when using 'complete' with directory names

[John Baldwin <jhb@FreeBSD.org>]

On 1/3/24 9:20 AM, Andrew Burgess wrote:
> This patch contains work pulled from this previously proposed patch:
> 
>    https://inbox.sourceware.org/gdb-patches/20210213220752.32581-2-lsix@lancelotsix.com/
> 
> But has been modified by me.  Credit for the original idea and
> implementation goes to Lancelot, any bugs in this new iteration belong
> to me.
> 
> Consider the executable `/tmp/foo/my_exec', and if we assume `/tmp' is
> empty other than the `foo' sub-directory, then currently within GDB,
> if I type:
> 
>    (gdb) file /tmp/f
> 
> and then hit TAB, GDB completes this to:
> 
>    (gdb) file /tmp/foo/
> 
> notice that not only did GDB fill in the whole of `foo', but GDB also
> added a trailing '/' character.  This is done within readline when the
> path that was just completed is a directory.  However, if I instead
> do:
> 
>    (gdb) complete file /tmp/f
>    file /tmp/foo
> 
> I now see the completed directory name, but the trailing '/' is
> missing.  The reason is that, in this case, the completions are not
> offered via readline, but are handled entirely within GDB, and so
> readline never gets the chance to add the trailing '/' character.
> 
> The above patch added filename option support to GDB, which included
> completion of the filename options.  This initially suffered from the
> same problem that I've outlined above, but the above patch proposed a
> solution to this problem, but this solution only applied to filename
> options (which have still not been added to GDB), and was mixed in
> with the complete filename options support.
> 
> This patch pulls out just the fix for the trailing "/" problem, and
> applies it to GDB's general filename completion.  This patch does not
> add filename options to GDB, that can always be done later, but I
> think this small part is itself a useful fix.
> 
> One of the biggest changes I made in this version is that I got rid of
> the set_from_readline member function, instead, I now pass the value
> of m_from_readline into the completion_tracker constructor.
> 
> I then moved the addition of the trailing '/' into filename_completer
> so that it is applied in the general filename completion case.  I also
> added a call to tilde_expand which was missing from the original
> patch, I haven't tested, but I suspect that this meant that the
> original patch would not add the trailing '/' if the user entered a
> path starting with a tilde character.
> 
> When writing the test for this patch I ran into two problems.
> 
> The first was that the procedures in lib/completion-support.exp relied
> on the command being completed for the test name.  This is fine for
> many commands, but not when completing a filename, if we use the
> command in this case the test name will (potentially) include the name
> of the directory in which the test is being run, which means we can't
> compare results between two runs of GDB from different directories.
> 
> So in this commit I've gone through completion-support.exp and added a
> new (optional) testname argument to many of the procedures, this
> allows me to give a unique test name, that doesn't include the path
> for my new tests.
> 
> The second issue was in the procedure make_tab_completion_list_re,
> this builds the completion list which is displayed after a double tab
> when there are multiple possible completions.
> 
> The procedure added the regexp ' +' after each completion, and then
> added another ' +' at the very end of the expected output.  So, if we
> expected to match the name of two functions 'f1' and 'f2' the
> generated regexp would be: 'f1 +f2 + +'.  This would match just fine,
> the actual output would be: 'f1  f2  ', notice that we get two spaces
> after each function name.
> 
> However, if we complete two directory names 'd1' and 'd2' then the
> output will be 'd1/ d2/ '.  Notice that now we only have a single
> space between each match, however, we do get the '/' added instead.
> 
> What happens is that when presenting the matches, readline always adds
> the appropriate trailing character; if we performed tab completion of
> 'break f1' then, as 'f1' is a unique match, we'd get 'break f1 ' with
> a trailing space added.  However, if we complete 'file d1' then we get
> 'file d1/'.  Then readline is adding a single space after each
> possible match, including the last one, which accounts for the
> trailing space character.
> 
> To resolve this I've simply remove the addition o the second ' +'
> within make_tab_completion_list_re, for the function completion
> example I gave above the expected pattern is now 'f1 +f2 +', which for
> the directory case we expect 'd1/ +d2/ +', both of which work just
> fine.
> 
> Co-Authored-By: Lancelot SIX <lsix@lancelotsix.com>
> ---
>   gdb/completer.c                               |  31 ++++-
>   gdb/completer.h                               |  12 +-
>   gdb/linespec.c                                |   2 +-
>   .../gdb.base/filename-completion.exp          |  94 ++++++++++++++
>   gdb/testsuite/lib/completion-support.exp      | 116 +++++++++++-------
>   5 files changed, 203 insertions(+), 52 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.base/filename-completion.exp
> 
> diff --git a/gdb/completer.c b/gdb/completer.c
> index d69ddcceca9..34f20bff83a 100644
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> @@ -225,6 +225,26 @@ 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 ())
> +	{
> +	  gdb::unique_xmalloc_ptr<char> expanded (tilde_expand (p_rl.get ()));
> +	  struct stat finfo;
> +	  const bool isdir
> +	    = stat (expanded.get (), &finfo) == 0 && S_ISDIR (finfo.st_mode);
> +	  if (isdir)
> +	    {
> +	      gdb::unique_xmalloc_ptr<char> tmp
> +		(static_cast<char *> (xmalloc (strlen (p_rl.get ()) + 2)));
> +	      char *next = stpcpy (tmp.get (), p_rl.get ());
> +	      strcpy (next, "/");
> +	      p_rl = std::move (tmp);
> +	    }
> +	}
> +
>         tracker.add_completion
>   	(make_completion_match_str (std::move (p_rl), text, word));
>       }

I think this might be more readable but perhaps slightly less efficient:

   if (isdir)
     {
       std::string tmp = string_printf ("%s/", p_rl.get ());
       p_rl = std::move (xstrdup (tmp.c_str ());
     }

(Can also use std::string for expanded with gdb_tilde_expand if you wanted)

-- 
John Baldwin