diff mbox series

[5/5] gdb: remove special case completion word handling for filenames

Message ID 048fa74bfb249273becb517d9edc9fd7a129e205.1705439706.git.aburgess@redhat.com
State New
Headers
Series Cleanup and changes for file name completion |

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 fail Testing failed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Andrew Burgess Jan. 16, 2024, 9:24 p.m. UTC 
  Eli,

CC-ing you directly as the work I'm touching was originally done by
you, see:

  https://sourceware.org/pipermail/gdb-patches/2001-February/007313.html

The original email mentions 4 cases:

  - completion on a file name in a list of file names didn't work;
  - GDB would not always append a slash if the completion is a directory;
  - completion failed when the file name had non-file-name characters,
    such as redirection, around it;
  - on DOS/Windows, completion would fail with files with a drive letter.

I believe I've tested the first 3 of these and they all seem to work
fine still, but I don't have a working Windows machine on which I can
test case #4.  I just wanted to bring this change to your attention in
case you wanted to build/test this and check that completion around
drive letters was still working as expected.

---

This commit removes some code which is special casing the filename
completion logic.  The code in question relates to finding the
beginning of the completion word and was first introduced, or modified
into its existing form in commit 7830cf6fb9571c3357b1a0 (from 2001).

The code being removed moved the start of the completion word backward
until a character in gdb_completer_file_name_break_characters was
found, or until we reached the end of the actual command.

However, I doubt that this is needed any more.  The filename completer
has a corresponding filename_completer_handle_brkchars function which
provides gdb_completer_file_name_break_characters as the word break
characters to readline, and also sets rl_completer_quote_characters.
As such, I would expect readline to be able to correctly find the
start of the completion word.

There is one change which I've needed to make as a consequence of
removing the above code, and I think this is a bug fix.

In complete_line_internal_normal_command we initialised temporary
variable P to the CMD_ARGS; this is the complete text after the
command name.  Meanwhile, complete_line_internal_normal_command also
accepts an argument WORD, which is the completion word that readline
found for us.

In the code I removed P was updated, it was first set to WORD, and
then moved backwards to the "new" start of the completion word.

But notice, the default for P is the complete command argument text,
and only if we are performing filename completion do we modify P to be
the completion word.

We then passed P through to the actual commands completion function.

If we are doing anything other than filename completion then the value
of P passed is the complete argument text.

If we are doing filename completion then the value of P passed is the
completion word.

Thus in filename_completer we get two arguments TEXT and WORD, the
TEXT argument gets the value of P which is the "new" completion word,
while WORD is the completion word that readline calculated.

However, after I removed the special case in
complete_line_internal_normal_command, the temporary P is removed, we
now always pass through the complete argument text where P was
previous used.

As such, filename_completer now receives TEXT as the complete argument
text, and WORD as the readline calculated completion word.

Previously in filename_completer we actually tried to generate
completions based on TEXT, which due to the special case, was the
completion word.  But after my change this is no longer the case.  So
I've updated filename_completer to generate completions based on WORD,
the readline calculated completion word.

If I'm correct, then I don't expect to see any user visible changes
after this commit.
---
 gdb/completer.c | 25 ++++---------------------
 1 file changed, 4 insertions(+), 21 deletions(-)

Comments

Eli Zaretskii Jan. 17, 2024, 12:09 p.m. UTC | #1 
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>,
> 	Eli Zaretskii <eliz@gnu.org>
> Date: Tue, 16 Jan 2024 21:24:02 +0000
> 
> Eli,
> 
> CC-ing you directly as the work I'm touching was originally done by
> you, see:
> 
>   https://sourceware.org/pipermail/gdb-patches/2001-February/007313.html
> 
> The original email mentions 4 cases:
> 
>   - completion on a file name in a list of file names didn't work;
>   - GDB would not always append a slash if the completion is a directory;
>   - completion failed when the file name had non-file-name characters,
>     such as redirection, around it;
>   - on DOS/Windows, completion would fail with files with a drive letter.
> 
> I believe I've tested the first 3 of these and they all seem to work
> fine still, but I don't have a working Windows machine on which I can
> test case #4.  I just wanted to bring this change to your attention in
> case you wanted to build/test this and check that completion around
> drive letters was still working as expected.

Thanks.  I won't have enough time for this soon enough, unfortunately.
Maybe someone else could do that?  I've taken the liberty of CC'ing
Hannes, in the hope that he could find some time for this soon.
Hannes Domani Jan. 17, 2024, 4:29 p.m. UTC | #2 
Am Mittwoch, 17. Januar 2024, 13:09:38 MEZ hat Eli Zaretskii <eliz@gnu.org> Folgendes geschrieben:

> > From: Andrew Burgess <aburgess@redhat.com>
>
> > Cc: Andrew Burgess <aburgess@redhat.com>,
> >     Eli Zaretskii <eliz@gnu.org>
> > Date: Tue, 16 Jan 2024 21:24:02 +0000
> >
> > Eli,
> >
> > CC-ing you directly as the work I'm touching was originally done by
> > you, see:
> >
> >  https://sourceware.org/pipermail/gdb-patches/2001-February/007313.html
> >
> > The original email mentions 4 cases:
> >
> >  - completion on a file name in a list of file names didn't work;
> >  - GDB would not always append a slash if the completion is a directory;
> >  - completion failed when the file name had non-file-name characters,
> >    such as redirection, around it;
> >  - on DOS/Windows, completion would fail with files with a drive letter.
> >
> > I believe I've tested the first 3 of these and they all seem to work
> > fine still, but I don't have a working Windows machine on which I can
> > test case #4.  I just wanted to bring this change to your attention in
> > case you wanted to build/test this and check that completion around
> > drive letters was still working as expected.
>
>
> Thanks.  I won't have enough time for this soon enough, unfortunately.
> Maybe someone else could do that?  I've taken the liberty of CC'ing
> Hannes, in the hope that he could find some time for this soon.


Not sure if this is exactly what you had in mind, but I tested
a few things with the patches applied, and saw no problems.

Absolute path:

(gdb) complete file c:/gdb/build64/gdb-git-python3/g
file c:/gdb/build64/gdb-git-python3/gdb/
file c:/gdb/build64/gdb-git-python3/gdbserver/
file c:/gdb/build64/gdb-git-python3/gdbsupport/
file c:/gdb/build64/gdb-git-python3/gnulib/
(gdb) complete file "c:/gdb/build64/gdb-git-python3/g
file "c:/gdb/build64/gdb-git-python3/gdb/"
file "c:/gdb/build64/gdb-git-python3/gdbserver/"
file "c:/gdb/build64/gdb-git-python3/gdbsupport/"
file "c:/gdb/build64/gdb-git-python3/gnulib/"

Absolute path on the same drive:

(gdb) complete file /gdb/build64/gdb-git-python3/g
file /gdb/build64/gdb-git-python3/gdb/
file /gdb/build64/gdb-git-python3/gdbserver/
file /gdb/build64/gdb-git-python3/gdbsupport/
file /gdb/build64/gdb-git-python3/gnulib/
(gdb) complete file "/gdb/build64/gdb-git-python3/g
file "/gdb/build64/gdb-git-python3/gdb/"
file "/gdb/build64/gdb-git-python3/gdbserver/"
file "/gdb/build64/gdb-git-python3/gdbsupport/"
file "/gdb/build64/gdb-git-python3/gnulib/"

Relative path:

(gdb) pwd
Working directory C:\gdb\build64\gdb-git-python3.
(gdb) complete file g
file gdb/
file gdbserver/
file gdbsupport/
file gnulib/
(gdb) complete file "g
file "gdb/"
file "gdbserver/"
file "gdbsupport/"
file "gnulib/"

And filename-completion.exp has no fails:

Running /c/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/filename-completion.exp ...

                === gdb Summary ===

# of expected passes            15
Eli Zaretskii Jan. 17, 2024, 4:52 p.m. UTC | #3 
> Date: Wed, 17 Jan 2024 16:29:39 +0000 (UTC)
> From: Hannes Domani <ssbssa@yahoo.de>
> Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
>  Am Mittwoch, 17. Januar 2024, 13:09:38 MEZ hat Eli Zaretskii <eliz@gnu.org> Folgendes geschrieben:
> 
> > Thanks.  I won't have enough time for this soon enough, unfortunately.
> > Maybe someone else could do that?  I've taken the liberty of CC'ing
> > Hannes, in the hope that he could find some time for this soon.
> 
> 
> Not sure if this is exactly what you had in mind, but I tested
> a few things with the patches applied, and saw no problems.

Thanks, I think this means file-name completion on Windows is unharmed
by Andrew's changes.
Andrew Burgess Jan. 18, 2024, 9:33 a.m. UTC | #4 
Hannes Domani <ssbssa@yahoo.de> writes:

>  Am Mittwoch, 17. Januar 2024, 13:09:38 MEZ hat Eli Zaretskii <eliz@gnu.org> Folgendes geschrieben:
>
>> > From: Andrew Burgess <aburgess@redhat.com>
>>
>> > Cc: Andrew Burgess <aburgess@redhat.com>,
>> >     Eli Zaretskii <eliz@gnu.org>
>> > Date: Tue, 16 Jan 2024 21:24:02 +0000
>> >
>> > Eli,
>> >
>> > CC-ing you directly as the work I'm touching was originally done by
>> > you, see:
>> >
>> >  https://sourceware.org/pipermail/gdb-patches/2001-February/007313.html
>> >
>> > The original email mentions 4 cases:
>> >
>> >  - completion on a file name in a list of file names didn't work;
>> >  - GDB would not always append a slash if the completion is a directory;
>> >  - completion failed when the file name had non-file-name characters,
>> >    such as redirection, around it;
>> >  - on DOS/Windows, completion would fail with files with a drive letter.
>> >
>> > I believe I've tested the first 3 of these and they all seem to work
>> > fine still, but I don't have a working Windows machine on which I can
>> > test case #4.  I just wanted to bring this change to your attention in
>> > case you wanted to build/test this and check that completion around
>> > drive letters was still working as expected.
>>
>>
>> Thanks.  I won't have enough time for this soon enough, unfortunately.
>> Maybe someone else could do that?  I've taken the liberty of CC'ing
>> Hannes, in the hope that he could find some time for this soon.
>
>
> Not sure if this is exactly what you had in mind, but I tested
> a few things with the patches applied, and saw no problems.

Thanks for doing this.  I didn't think this would have broken anything,
but without a Windows machine to test on I'm always a little nervous
messing with this sort of code.  I feel better knowing that at least
I've not completely broken things :)

Thanks again,
Andrew

>
> Absolute path:
>
> (gdb) complete file c:/gdb/build64/gdb-git-python3/g
> file c:/gdb/build64/gdb-git-python3/gdb/
> file c:/gdb/build64/gdb-git-python3/gdbserver/
> file c:/gdb/build64/gdb-git-python3/gdbsupport/
> file c:/gdb/build64/gdb-git-python3/gnulib/
> (gdb) complete file "c:/gdb/build64/gdb-git-python3/g
> file "c:/gdb/build64/gdb-git-python3/gdb/"
> file "c:/gdb/build64/gdb-git-python3/gdbserver/"
> file "c:/gdb/build64/gdb-git-python3/gdbsupport/"
> file "c:/gdb/build64/gdb-git-python3/gnulib/"
>
> Absolute path on the same drive:
>
> (gdb) complete file /gdb/build64/gdb-git-python3/g
> file /gdb/build64/gdb-git-python3/gdb/
> file /gdb/build64/gdb-git-python3/gdbserver/
> file /gdb/build64/gdb-git-python3/gdbsupport/
> file /gdb/build64/gdb-git-python3/gnulib/
> (gdb) complete file "/gdb/build64/gdb-git-python3/g
> file "/gdb/build64/gdb-git-python3/gdb/"
> file "/gdb/build64/gdb-git-python3/gdbserver/"
> file "/gdb/build64/gdb-git-python3/gdbsupport/"
> file "/gdb/build64/gdb-git-python3/gnulib/"
>
> Relative path:
>
> (gdb) pwd
> Working directory C:\gdb\build64\gdb-git-python3.
> (gdb) complete file g
> file gdb/
> file gdbserver/
> file gdbsupport/
> file gnulib/
> (gdb) complete file "g
> file "gdb/"
> file "gdbserver/"
> file "gdbsupport/"
> file "gnulib/"
>
> And filename-completion.exp has no fails:
>
> Running /c/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/filename-completion.exp ...
>
>                 === gdb Summary ===
>
> # of expected passes            15
diff mbox series

Patch

diff --git a/gdb/completer.c b/gdb/completer.c
index 9c89aa43810..b78946d66dd 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -210,7 +210,7 @@  filename_completer (struct cmd_list_element *ignore,
   while (1)
     {
       gdb::unique_xmalloc_ptr<char> p_rl
-	(rl_filename_completion_function (text, subsequent_name));
+	(rl_filename_completion_function (word, subsequent_name));
       if (p_rl == NULL)
 	break;
       /* We need to set subsequent_name to a non-zero value before the
@@ -239,7 +239,7 @@  filename_completer (struct cmd_list_element *ignore,
 	}
 
       tracker.add_completion
-	(make_completion_match_str (std::move (p_rl), text, word));
+	(make_completion_match_str (std::move (p_rl), word, word));
     }
 }
 
@@ -1193,23 +1193,6 @@  complete_line_internal_normal_command (completion_tracker &tracker,
 				       complete_line_internal_reason reason,
 				       struct cmd_list_element *c)
 {
-  const char *p = cmd_args;
-
-  if (c->completer == filename_completer)
-    {
-      /* Many commands which want to complete on file names accept
-	 several file names, as in "run foo bar >>baz".  So we don't
-	 want to complete the entire text after the command, just the
-	 last word.  To this end, we need to find the beginning of the
-	 file name by starting at `word' and going backwards.  */
-      for (p = word;
-	   p > command
-	     && strchr (gdb_completer_file_name_break_characters,
-			p[-1]) == NULL;
-	   p--)
-	;
-    }
-
   if (reason == handle_brkchars)
     {
       completer_handle_brkchars_ftype *brkchars_fn;
@@ -1223,11 +1206,11 @@  complete_line_internal_normal_command (completion_tracker &tracker,
 	       (c->completer));
 	}
 
-      brkchars_fn (c, tracker, p, word);
+      brkchars_fn (c, tracker, cmd_args, word);
     }
 
   if (reason != handle_brkchars && c->completer != NULL)
-    (*c->completer) (c, tracker, p, word);
+    (*c->completer) (c, tracker, cmd_args, word);
 }
 
 /* Internal function used to handle completions.