Re: [PATCH PR gdb/15715] 'set history filename' to by immediately converted to absolute path.

[Pedro Alves <palves@redhat.com>]

On 07/12/2013 12:05 PM, Bilal, Muhammad wrote:

> Please find attached updated patch with new PR in gdb bugzilla.
> A workaround for this patch has been discussed here, 

You meant background, right?

> http://sourceware.org/ml/gdb-patches/2013-05/msg00852.html

> gdb/ChangeLog 
> 2013-07-13  Muhammad Bilal  <mbilal@codesorcery.com>
> 
>     PR gdb/15715
>     * top.c (set_history_filename): New function.
>     Converted history filename path to its absolute path.

This is still incomplete.  You'll need to mention the inclusion
of filenames.h, and when I said "you must have done something
with the function", I meant that something must be calling the
function.  So:

    * top.c: Include "filenames.h".
   (set_history_filename): New function.
   (init_main): Install it as set hook of the "set history filename"
   command.

> gdb/testsuite/ChangeLog
> 2013-07-13  Muhammad Bilal  <mbilal@codesourcery.com>
> 
>     PR gdb/15715
>     * gdb.base/setshow.exp: Test 'set history filename' command
>     to check its absolute path.

Write instead:

gdb/testsuite/ChangeLog
2013-07-13  Muhammad Bilal  <mbilal@codesourcery.com>
 
     PR gdb/15715
     * gdb.base/setshow.exp: Test that relative paths passed to
     'set history filename' are converted to absolute paths.

>  static void
> +set_history_filename (char *args, int from_tty, struct cmd_list_element *c)
> +{
> +  /*We include the current directory so that if the user changes    
> +    directories the file written will be the same as the one 
> +    that was read.  */

There should be a space after '/*' (and then realign the following lines).

>  static void
> +set_history_filename (char *args, int from_tty, struct cmd_list_element *c)
> +{
> +  /*We include the current directory so that if the user changes    
> +    directories the file written will be the same as the one 
> +    that was read.  */

> +  if (!IS_ABSOLUTE_PATH (history_filename))
> +    history_filename = reconcat (history_filename ,current_directory, "/", history_filename,
> +				 (char *) NULL);

Wrong formatting.  Line too long, spaces around ','.  Please check
the GNU coding standards.


> Index: gdb/top.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/top.c,v
> retrieving revision 1.238
> diff -u -p -r1.238 top.c
> --- gdb/top.c	6 Jul 2013 07:34:48 -0000	1.238
> +++ gdb/top.c	12 Jul 2013 10:43:04 -0000
> @@ -48,6 +48,7 @@
>  #include "interps.h"
>  #include "observer.h"
>  #include "maint.h"
> +#include "filenames.h"
>  
>  /* readline include files.  */
>  #include "readline/readline.h"
> @@ -1698,6 +1699,17 @@ set_gdb_datadir (char *args, int from_tt
>  }
>  
>  static void
> +set_history_filename (char *args, int from_tty, struct cmd_list_element *c)
> +{
> +  /*We include the current directory so that if the user changes    
> +    directories the file written will be the same as the one 
> +    that was read.  */
> +  if (!IS_ABSOLUTE_PATH (history_filename))
> +    history_filename = reconcat (history_filename ,current_directory, "/", history_filename,
> +				 (char *) NULL);
> +}
> +
> +static void
>  init_main (void)
>  {
>    /* Initialize the prompt to a simple "(gdb) " prompt or to whatever
> @@ -1773,7 +1785,7 @@ variable \"HISTSIZE\", or to 256 if this
>  Set the filename in which to record the command history"), _("\
>  Show the filename in which to record the command history"), _("\
>  (the list of previous commands of which a record is kept)."),
> -			    NULL,
> +			    set_history_filename,
>  			    show_history_filename,
>  			    &sethistlist, &showhistlist);
> Index: gdb/testsuite/gdb.base/setshow.exp
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.base/setshow.exp,v
> retrieving revision 1.33
> diff -u -p -r1.33 setshow.exp
> --- gdb/testsuite/gdb.base/setshow.exp	27 Jun 2013 18:58:28 -0000	1.33
> +++ gdb/testsuite/gdb.base/setshow.exp	12 Jul 2013 10:44:36 -0000
> @@ -165,11 +165,36 @@ gdb_test_no_output "set height unlimited
>  gdb_test_no_output "set history expansion on" "set history expansion on" 
>  #test show history expansion on
>  gdb_test "show history expansion on" "History expansion on command input is on.*" "show history expansion" 
> +#get home directory path
> +set HOME ""
> +gdb_test_multiple "show environment HOME" "show home directory" {
> +    -re "\nHOME = (.*).\n.*" {
> +        set HOME $expect_out(1,string)
> +        pass "show home directory"
> +    }
> +}

I'd suggest:

set HOME ""
set test "show environment HOME"
gdb_test_multiple $test $test {
    -re "\nHOME = (.*).\n.*" {
        set HOME $expect_out(1,string)
        pass $test
    }
}

> +#test set history filename ~/foobar.baz
> +gdb_test_no_output "set history filename ~/foobar.baz" \
> +    "set history filename ~/foobar.baz"
> +#test show history filename ~/foobar.baz
> +gdb_test "show history filename" \
> +    "The filename in which to record the command history is \"$HOME/foobar.baz\"..*" \
> +    "show history filename $HOME/foobar.baz"

The expansion of $HOME is being placed in the test message
(what goes to gdb.sum after PASS/FAIL).  That means that the
test message depends on who/where you run it, which is bad
for comparing test results.

> +#get current working directory
> +set PWD ""
> +gdb_test_multiple "pwd" "show working directory" {
> +    -re "\nWorking directory (.*)..\n.*" {
> +        set PWD $expect_out(1,string)
> +        pass "show working directory"
> +    }
> +}
>  #test set history filename foobar.baz
>  gdb_test_no_output "set history filename foobar.baz" \
> -    "set history filename foobar.baz" 
> +    "set history filename foobar.baz"
>  #test show history filename foobar.baz
> -gdb_test "show history filename" "The filename in which to record the command history is \"foobar.baz\"..*"  "show history filename (foobar.baz)" 
> +gdb_test "show history filename" \
> +    "The filename in which to record the command history is \"$PWD/foobar.baz\"..*" \
> +    "show history filename $PWD\foobar.baz"

Likewise.  Use forward slash, not backslash.

>  #test set history save on
>  gdb_test_no_output "set history save on" "set history save on" 
>  #test show history save on
> 

-- 
Pedro Alves