Re: Use external editor in 'commands' command

[Alfredo Ortega <ortegaalfredo at gmail dot com>]

Hi all,

Sorry Tom for the very late answer. But I think I got all your
recommendations implemented on the patch.
Except that I couldn't find a way to do a cleanup erasing a file,
without resorting to the ui_file functions.
But that wouldn't work because I need a *FILE for instream. Any hint?
Sending my latest patch anyway if someone want to take a look at it.
Regards,

Alfred


2009-08-28 Alfredo Ortega <ortegaalfredo@gmail.com>
 * breakpoint.c (commands_command,_initialize_breakpoint,
edit_command,check_executing_commands, commands_edit_command,
commands_dump_to_file):
Add the 'edit' keyword to the 'commands' command.
 * utils.c,defs.h (external_editor,initialize_utils):
 Added an utility function to return the external text editor of the system.
 Added new "set external-editor" and "show external-editor" commands.

2009-08-28 Alfredo Ortega <ortegaalfredo@gmail.com>
 * gdb.texinfo, refcard.tex (breakpoint commands, set
external-editor, show external-editor): Added
documentation of the edit option for editing commands.



2009/2/24 Tom Tromey <tromey@redhat.com>:
>>>>>> "Alfredo" == Alfredo Ortega <ortegaalfredo@gmail.com> writes:
>
> Alfredo> 2009-01-16 Alfredo Ortega <ortegaalfredo@gmail.com>
> Alfredo> ? * breakpoint.c (commands_command,_initialize_breakpoint,
> Alfredo> edit_command,check_executing_commands, commands_edit_command,
> Alfredo> commands_dump_to_file):
> Alfredo> ? Add the 'edit' keyword to the 'commands' command to allow the
> Alfredo> ? use of an external editor to add or modify commands.
> Alfredo> ? Added an utility function to dump breakpoint commands to a file.
> Alfredo> ? Added an utility function to launch an external editor on breakpoint commands.
> Alfredo> ? Joined common checks in commands_command and edit_command.
>
> This is wordier than the norm. ?Also, a comment describing a change
> should be next to the name of the function it describes. ?This seems
> to list all the function names, then all the changes.
>
> See the existing ChangeLog for a lot of examples. ?Also the GNU
> standards have a section on writing a ChangeLog entry...
>
> Alfredo> +commands_dump_to_file (char *filename, struct breakpoint *b)
> Alfredo> +{
> Alfredo> + ?struct cleanup *cleanups;
> Alfredo> + ?struct ui_file *old, *outfile = gdb_fopen (filename, "w");
>
> You have to check for a NULL return from gdb_fopen.
>
> Alfredo> + ?do_cleanups (cleanups);
> Alfredo> +
> Alfredo> +}
>
> Extra blank line in there.
>
> Alfredo> +/* Launches the editor on the breakpoint command. ?*/
> Alfredo> +char *
> Alfredo> +commands_edit_command (int bnum)
>
> The header comment should describe the meaning of the return value and
> the arguments. ?For a string return, like this, it should also
> describe how the memory is managed.
>
> Alfredo> + ?if (!vitmp)
> Alfredo> + ? ? ?error (_("Can't create temporary file for editing."));
>
> Two extra spaces of indentation on the second line, not four. ?This
> occurs in a few spots.
>
> Alfredo> + ?ALL_BREAKPOINTS (b) if (b->number == bnum)
>
> The 'if' should go on a new line.
> This will require some reindentation, as well.
>
> Alfredo> + ? ? ?cmdline = xmalloc (strlen (editor) + strlen (vitmp) + 50);
>
> Instead of 50, just use the correct number.
> Or, use xstrprintf, which is simpler.
>
> Alfredo> + ? ? ?if (sysret < 0)
> Alfredo> + ? ? ? ?error (_("Can't execute external editor."));
> Alfredo> + ? ? ?if (sysret > 0)
> Alfredo> + ? ? ? ?error (_("External editor returned non-zero status."));
>
> I think you need to use WEXITSTATUS and friends here.
>
> Alfredo> + ?ALL_BREAKPOINTS (b) if (b->number == bnum)
>
> Newline.
>
> Alfredo> + ? ?{
> Alfredo> + ? ? ?/* Redirect instream to the commands temporal file. ?*/
>
> I think this can be just "Read commands from the temporary file."
>
> Alfredo> + ? ? ?instream = fopen (vitmp, "r");
>
> Needs error checking.
>
> Alfredo> + ? ? ?unlink (vitmp);
>
> I suspect this should probably be done via a cleanup.
>
> Alfredo> +/* List of subcommands for "edit". ?*/
> Alfredo> +static struct cmd_list_element *edit_command_list;
>
> I don't think you need this...
>
> Alfredo> + ?add_prefix_cmd ("edit", class_breakpoint, edit_command,_("\
>
> ... because I don't think "edit" needs to be a prefix command.
> It can just be an ordinary command.
>
> Alfredo> +extern char *external_editor( void );
>
> Should be " (void)", not "( void )".
>
> Alfredo> + ?add_setshow_filename_cmd ("external-editor", no_class, &external_editor_command, _("\
>
> I don't remember... did we already discuss the name of the option?
> Why "external-editor" and not the simpler "editor"?
>
> Alfredo> +/* Returns the external editor */
>
> Needs period and 2 spaces at the end.
>
> Alfredo> + ?return editor;
> Alfredo> +
> Alfredo> +}
>
> Extra blank line.
>
> This is pretty close to ready. ?Thanks for persevering.
>
> Tom
>

diff -u -p -r1.418 breakpoint.c
--- gdb/breakpoint.c	21 Aug 2009 18:54:44 -0000	1.418
+++ gdb/breakpoint.c	28 Aug 2009 22:11:05 -0000
@@ -47,6 +47,7 @@
 #include "completer.h"
 #include "gdb.h"
 #include "ui-out.h"
+#include "cli-out.h"
 #include "cli/cli-script.h"
 #include "gdb_assert.h"
 #include "block.h"
@@ -652,20 +653,129 @@ breakpoint_set_commands (struct breakpoi
   observer_notify_breakpoint_modified (b->number);
 }
 
+/* Dumps breakpoint commands to a file.  */
 static void
-commands_command (char *arg, int from_tty)
+commands_dump_to_file (char *filename, struct breakpoint *b)
 {
+  struct cleanup *cleanups;
+  struct ui_file *old, *outfile = gdb_fopen (filename, "w");
+  if (outfile==NULL)
+      error (_("Can't create temporary file for editing."));
+  cleanups = make_cleanup_ui_file_delete (outfile);
+  old = cli_out_set_stream (uiout, outfile);
+  print_command_lines (uiout, b->commands, 4);
+  cli_out_set_stream (uiout, old);
+  do_cleanups (cleanups);
+}
+
+/* Launches the editor on the breakpoint command.  
+   bnum is the breakpoint number.
+   Returns the name of a temporary file containing
+   the edited commands. The returned string must be freed. */
+char *
+commands_edit_command (int bnum)
+{
+  char *vitmp = NULL;
+  char *editor;
   struct breakpoint *b;
-  char *p;
-  int bnum;
-  struct command_line *l;
+  char *cmdline = NULL;
+  int sysret;
+  /* Generates the temporary file name.  */
+  vitmp = make_temp_file (NULL);
+  if (!vitmp)
+    error (_("Can't create temporary file for editing."));
+  editor = external_editor ();
+  if (!editor)
+    error (_("External editor not found."));
+  ALL_BREAKPOINTS (b)
+    if (b->number == bnum)
+      {
+        if (&b->commands)
+	  commands_dump_to_file (vitmp, b);
+        /* Edit the file.  */
+	cmdline = xstrprintf("%s \"%s\"",editor,vitmp);
+        sysret = system (cmdline);
+        xfree (cmdline);
+        if (sysret < 0)
+  	  error (_("Can't execute external editor."));
+        if (WEXITSTATUS(sysret) > 0)
+  	  error (_("External editor returned non-zero status."));
+      }
+  return vitmp;
+}
 
   /* If we allowed this, we would have problems with when to
      free the storage, if we change the commands currently
      being read from.  */
 
+static void
+check_executing_commands ()
+{
   if (executing_breakpoint_commands)
     error (_("Can't use the \"commands\" command among a breakpoint's commands."));
+}
+
+/* Like commands_command but using an external editor.  */
+static void
+edit_command (char *args, int from_tty)
+{
+  int bnum;
+  char *p;
+  char *vitmp;
+  struct breakpoint *b;
+  struct command_line *l;
+  FILE *tmpstream = NULL;
+  struct cleanup *cleanups;
+
+  check_executing_commands ();
+  p = args;
+  if (!p)
+    error (_("No breakpoint number."));
+  bnum = get_number (&p);
+  if (p && *p)
+    error (_("Unexpected extra arguments following breakpoint number."));
+  /* Launches the editor.  */
+  vitmp = commands_edit_command (bnum);
+  if (!vitmp)
+    error (_("Can't create temporary file for editing."));
+  cleanups = make_cleanup(xfree,vitmp);
+
+  ALL_BREAKPOINTS (b)
+    if (b->number == bnum)
+      {
+        /* Read commands from the temporary file.  */
+        tmpstream = instream;
+        instream = fopen (vitmp, "r");
+	if (instream==NULL)
+	  {
+	  instream=tmpstream;
+    	  error (_("Can't open commands temporary file."));
+	  }
+        l = read_command_lines (NULL, from_tty, 1);
+        free_command_lines (&b->commands);
+        b->commands = l;
+        breakpoints_changed ();
+        observer_notify_breakpoint_modified (b->number);
+        /* Restore instream and erase temporal file.  */
+        instream = tmpstream;
+	unlink(vitmp);
+  	do_cleanups (cleanups);
+        return;
+      }
+  unlink(vitmp);
+  do_cleanups (cleanups);
+  error (_("No breakpoint number %d."), bnum);
+}
+
+static void
+commands_command (char *arg, int from_tty)
+{
+  struct breakpoint *b;
+  char *p;
+  int bnum;
+  struct command_line *l;
+
+  check_executing_commands();
 
   p = arg;
   bnum = get_number (&p);
@@ -8891,6 +9001,9 @@ Multiple breakpoints at one place are pe
 \n\
 Do \"help breakpoints\" for info on other commands dealing with breakpoints."
 
+/* List of subcommands for "commands".  */
+static struct cmd_list_element *commands_command_list;
+
 /* List of subcommands for "catch".  */
 static struct cmd_list_element *catch_cmdlist;
 
@@ -8941,14 +9054,20 @@ Usage is `ignore N COUNT'."));
   if (xdb_commands)
     add_com_alias ("bc", "ignore", class_breakpoint, 1);
 
-  add_com ("commands", class_breakpoint, commands_command, _("\
+  add_prefix_cmd ("commands", class_breakpoint, commands_command, _("\
 Set commands to be executed when a breakpoint is hit.\n\
 Give breakpoint number as argument after \"commands\".\n\
 With no argument, the targeted breakpoint is the last one set.\n\
 The commands themselves follow starting on the next line.\n\
 Type a line containing \"end\" to indicate the end of them.\n\
 Give \"silent\" as the first line to make the breakpoint silent;\n\
-then no output is printed when it is hit, except what the commands print."));
+then no output is printed when it is hit, except what the commands print."),
+		      &commands_command_list, "commands ", 1, &cmdlist);
+
+  add_cmd ("edit", class_breakpoint, edit_command,_("\
+Edit the command using the external-editor variable, EDITOR environment\n\
+variable or /bin/ex, in that precedence."),
+		      &commands_command_list);
 
   add_com ("condition", class_breakpoint, condition_command, _("\
 Specify breakpoint number N to break only if COND is true.\n\
Index: gdb/defs.h
===================================================================
RCS file: /cvs/src/src/gdb/defs.h,v
retrieving revision 1.254
diff -u -p -r1.254 defs.h
--- gdb/defs.h	3 Aug 2009 12:26:37 -0000	1.254
+++ gdb/defs.h	28 Aug 2009 22:11:06 -0000
@@ -335,6 +335,8 @@ extern int subset_compare (char *, char 
 
 extern char *safe_strerror (int);
 
+extern char *external_editor(void);
+
 #define	ALL_CLEANUPS	((struct cleanup *)0)
 
 extern void do_cleanups (struct cleanup *);
Index: gdb/utils.c
===================================================================
RCS file: /cvs/src/src/gdb/utils.c,v
retrieving revision 1.219
diff -u -p -r1.219 utils.c
--- gdb/utils.c	18 Aug 2009 16:17:16 -0000	1.219
+++ gdb/utils.c	28 Aug 2009 22:11:07 -0000
@@ -98,6 +98,10 @@ static void prompt_for_continue (void);
 static void set_screen_size (void);
 static void set_width (void);
 
+/* External text editor */
+
+static char *external_editor_command = NULL;
+
 /* A flag indicating whether to timestamp debugging messages.  */
 
 static int debug_timestamp = 0;
@@ -2834,6 +2838,12 @@ When set, debugging messages will be mar
 			   NULL,
 			   show_debug_timestamp,
 			   &setdebuglist, &showdebuglist);
+  add_setshow_filename_cmd ("external-editor", no_class, &external_editor_command, _("\
+Set the external text editor that gdb uses."),
+			    _("\
+Show the external text editor."), NULL,
+			    NULL, NULL,
+			    &setlist, &showlist);
 }
 
 /* Machine specific function to handle SIGWINCH signal. */
@@ -3563,3 +3573,15 @@ _initialize_utils (void)
   add_internal_problem_command (&internal_error_problem);
   add_internal_problem_command (&internal_warning_problem);
 }
+
+/* Returns the external editor.  */
+char *
+external_editor (void)
+{
+  char *editor;
+  if (external_editor_command)
+    return external_editor_command;
+  if ((editor = (char *) getenv ("EDITOR")) == NULL)
+    editor = "/bin/ex";
+  return editor;
+}
Index: gdb/doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.618
diff -u -p -r1.618 gdb.texinfo
--- gdb/doc/gdb.texinfo	27 Aug 2009 21:56:38 -0000	1.618
+++ gdb/doc/gdb.texinfo	28 Aug 2009 22:11:17 -0000
@@ -4024,6 +4024,8 @@ follow it immediately with @code{end}; t
 With no @var{bnum} argument, @code{commands} refers to the last
 breakpoint, watchpoint, or catchpoint set (not to the breakpoint most
 recently encountered).
+@item commands edit @r{[}@var{bnum}@r{]}
+This spawns an external editor for adding or editing commands.  The final  @code{end} is not necessary in this case.  @xref{Choosing your Editor}.
 @end table
 
 Pressing @key{RET} as a means of repeating the last @value{GDBN} command is
@@ -5675,6 +5677,7 @@ prefer to use Emacs facilities to view s
 * List::                        Printing source lines
 * Specify Location::            How to specify code locations
 * Edit::                        Editing source files
+* Choosing your Editor::        Specifying your text editor
 * Search::                      Searching source files
 * Source Path::                 Specifying source directories
 * Machine Code::                Source and machine code
@@ -5878,6 +5881,7 @@ Edit the file containing @var{function} 
 
 @end table
 
+@node Choosing your Editor
 @subsection Choosing your Editor
 You can customize @value{GDBN} to use any editor you want
 @footnote{
@@ -5902,6 +5906,19 @@ or in the @code{csh} shell,
 setenv EDITOR /usr/bin/vi
 gdb @dots{}
 @end smallexample
+Another option is to use the @code{set external-editor} command:
+
+@table @code
+@item set external-editor
+@kindex set external-editor
+This command controls the external text editor that internal gdb commands use. The external-editor variable has precedence over the @code{EDITOR} enviroment variable.
+@end table
+
+@table @code
+@item show external-editor
+@kindex show external-editor
+This command shows the external text editor that internal gdb commands use. 
+@end table
 
 @node Search
 @section Searching Source Files
Index: gdb/doc/refcard.tex
===================================================================
RCS file: /cvs/src/src/gdb/doc/refcard.tex,v
retrieving revision 1.6
diff -u -p -r1.6 refcard.tex
--- gdb/doc/refcard.tex	27 Mar 2007 18:09:35 -0000	1.6
+++ gdb/doc/refcard.tex	28 Aug 2009 22:11:17 -0000
@@ -355,10 +355,9 @@ delete when reached
 ignore {\it n} {\it count}&ignore breakpoint {\it n}, {\it count}
 times\cr
 \cr
-commands {\it n}\par
+commands \opt{{\it edit}} {\it n}\par
 \qquad \opt{\tt silent}\par
-\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached. \opt{{\tt silent} suppresses default
-display}\cr
+\qquad {\it command-list}&execute GDB {\it command-list} every time breakpoint {\it n} is reached  \opt{{\tt edit} using external editor}. \opt{{\tt silent} suppresses default display} \cr
 end&end of {\it command-list}\cr
 \endsec