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

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

[Bilal, Muhammad]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

Hi,

Please find attached updated patch with new PR in gdb bugzilla.
A workaround for this patch has been discussed here, 
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.

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.



Thanks,

-Bilal








[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: pr15715.diff --]
[-- Type: text/x-patch; name="pr15715.diff", Size: 3487 bytes --]

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"
+    }
+}
+#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"
+#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"
 #test set history save on
 gdb_test_no_output "set history save on" "set history save on" 
 #test show history save on

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

[Muhammad Bilal]

On 07/12/2013 04:05 PM, Bilal, Muhammad wrote:
> Hi,
>
> Please find attached updated patch with new PR in gdb bugzilla.
> A workaround for this patch has been discussed here,
> 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.
>
> 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.
>
>
>
> Thanks,
>
> -Bilal
>
>
>
>
>
>
>
ping

Thanks
-Bilal

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

[Muhammad Bilal]

On 07/19/2013 03:22 PM, Muhammad Bilal wrote:
> On 07/12/2013 04:05 PM, Bilal, Muhammad wrote:
>> Hi,
>>
>> Please find attached updated patch with new PR in gdb bugzilla.
>> A workaround for this patch has been discussed here,
>> 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.
>>
>> 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.
>>
>>
>>
>> Thanks,
>>
>> -Bilal
>>
>>
>>
>>
>>
>>
>>
> ping
>
> Thanks
> -Bilal
ping?

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

[Pedro Alves]

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

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

[Muhammad Bilal]

[-- Attachment #1: Type: text/plain, Size: 7135 bytes --]

Hi Pedro,

Thanks for review and help to make the ChangeLog entry.
please find attached updated patch.


./ChangeLog

2013-07-29  Muhammad Bilal  <mbilal@codesorcery.com>

         PR gdb/15715
         * top.c: include "filenames.h"
         (set_history_filename):New function.
         (init_main): Install it as set hook of the "set history filename"
         command.
./testsuite/ChangeLog

2013-07-29  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.


On 07/26/2013 05:49 PM, Pedro Alves wrote:
> 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
>>





Thanks,
-Bilal

[-- Attachment #2: pr15715.diff --]
[-- Type: text/x-patch, Size: 3482 bytes --]

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	29 Jul 2013 05:55:40 -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	29 Jul 2013 05:57:23 -0000
@@ -165,11 +165,38 @@ 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 ""
+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 (~/foobar.baz)"
+#get current working directory
+set PWD ""
+set test "show working directory"
+gdb_test_multiple "pwd" $test {
+    -re "\nWorking directory (.*)..\n.*" {
+        set PWD $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" 
+    "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 (current_directory/foobar.baz)"
 #test set history save on
 gdb_test_no_output "set history save on" "set history save on" 
 #test show history save on

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

[Pedro Alves]

Hello,

On 07/29/2013 07:12 AM, Muhammad Bilal wrote:

> 2013-07-29  Muhammad Bilal  <mbilal@codesorcery.com>
> 
>          PR gdb/15715
>          * top.c: include "filenames.h"
>          (set_history_filename):New function.

Space after ':'.

>  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.  */

Formatting still not right.  Add a space after '/*', and then reindent.
Also, there's trailing whitespace in the first two lines that should not
be there.  The correct format is:

    /* 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);
> +}

Thanks,
-- 
Pedro Alves

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

[Muhammad Bilal]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]

Oh , my bad, and I am  sorry

On 07/29/2013 07:20 PM, Pedro Alves wrote:
> Hello,
>
> On 07/29/2013 07:12 AM, Muhammad Bilal wrote:
>
>> 2013-07-29  Muhammad Bilal  <mbilal@codesorcery.com>
>>
>>           PR gdb/15715
>>           * top.c: include "filenames.h"
>>           (set_history_filename):New function.
> Space after ':'.
fixed.
>>   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.  */
> Formatting still not right.  Add a space after '/*', and then reindent.
> Also, there's trailing whitespace in the first two lines that should not
> be there.  The correct format is:
>
>      /* 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);
>> +}
> Thanks,

fixed.

Please find new patch.


2013-07-30  Muhammad Bilal  <mbilal@codesorcery.com>

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

./testsuit

2013-07-30  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.






OK?




Thanks,
-Bilal






[-- Attachment #2: pr15715.diff --]
[-- Type: text/x-patch, Size: 3486 bytes --]

Index: gdb/top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.239
diff -u -p -r1.239 top.c
--- gdb/top.c	22 Jul 2013 11:42:31 -0000	1.239
+++ gdb/top.c	30 Jul 2013 09:39:50 -0000
@@ -48,6 +48,7 @@
 #include "interps.h"
 #include "observer.h"
 #include "maint.h"
+#include "filenames.h"
 
 /* readline include files.  */
 #include "readline/readline.h"
@@ -1704,6 +1705,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
@@ -1779,7 +1791,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	30 Jul 2013 09:46:44 -0000
@@ -165,11 +165,38 @@ 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 ""
+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 (~/foobar.baz)"
+#get current working directory
+set PWD ""
+set test "show working directory"
+gdb_test_multiple "pwd" $test {
+    -re "\nWorking directory (.*)..\n.*" {
+        set PWD $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" 
+    "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 (current_directory/foobar.baz)"
 #test set history save on
 gdb_test_no_output "set history save on" "set history save on" 
 #test show history save on

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

[Pedro Alves]

On 07/30/2013 10:56 AM, Muhammad Bilal wrote:
> On 07/29/2013 07:20 PM, Pedro Alves wrote:

>>> >>   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.  */
>> > Formatting still not right.  Add a space after '/*', and then reindent.
>> > Also, there's trailing whitespace in the first two lines that should not
>> > The correct format is:
>> >
>> >     /* 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.  */

...

> 2013-07-30  Muhammad Bilal  <mbilal@codesorcery.com>
> 
>          PR gdb/15715
>          * top.c: include "filenames.h"

My initial suggestion was:

          * top.c: Include "filenames.h".

That is, uppercase "include", and finish sentence with period.
Please fix this.

>  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. */

The trailing whitespace in first two comment lines is still there.  And
this lost the double space after period; please add it back.  Or just
copy the right formatting I pasted in the previous email...

-- 
Pedro Alves

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

[Muhammad Bilal]

[-- Attachment #1: Type: text/plain, Size: 2164 bytes --]

Hi Pedro,

On 07/30/2013 03:09 PM, Pedro Alves wrote:
> On 07/30/2013 10:56 AM, Muhammad Bilal wrote:
>> On 07/29/2013 07:20 PM, Pedro Alves wrote:
>>>>>>    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.  */
>>>> Formatting still not right.  Add a space after '/*', and then reindent.
>>>> Also, there's trailing whitespace in the first two lines that should not
>>>> The correct format is:
>>>>
>>>>      /* 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.  */
> ...
>
>> 2013-07-30  Muhammad Bilal  <mbilal@codesorcery.com>
>>
>>           PR gdb/15715
>>           * top.c: include "filenames.h"
> My initial suggestion was:
>
>            * top.c: Include "filenames.h".
>
> That is, uppercase "include", and finish sentence with period.
> Please fix this.
>
>>   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. */
> The trailing whitespace in first two comment lines is still there.  And
> this lost the double space after period; please add it back.  Or just
> copy the right formatting I pasted in the previous email...
>

2012-07-30  Muhammad Bilal  <mbilal@codesorcery.com>

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

/testsuit
2012-07-30  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

Please find new one.

I think in ChangeLog, there should be only you name, not mine :)


Thanks,
-Bilal




[-- Attachment #2: pr15715.diff --]
[-- Type: text/x-patch, Size: 3482 bytes --]

Index: gdb/top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.239
diff -u -p -r1.239 top.c
--- gdb/top.c	22 Jul 2013 11:42:31 -0000	1.239
+++ gdb/top.c	30 Jul 2013 10:38:02 -0000
@@ -48,6 +48,7 @@
 #include "interps.h"
 #include "observer.h"
 #include "maint.h"
+#include "filenames.h"
 
 /* readline include files.  */
 #include "readline/readline.h"
@@ -1704,6 +1705,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
@@ -1779,7 +1791,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	30 Jul 2013 09:46:44 -0000
@@ -165,11 +165,38 @@ 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 ""
+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 (~/foobar.baz)"
+#get current working directory
+set PWD ""
+set test "show working directory"
+gdb_test_multiple "pwd" $test {
+    -re "\nWorking directory (.*)..\n.*" {
+        set PWD $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" 
+    "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 (current_directory/foobar.baz)"
 #test set history save on
 gdb_test_no_output "set history save on" "set history save on" 
 #test show history save on

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

[Pedro Alves]

On 07/30/2013 11:46 AM, Muhammad Bilal wrote:

> 2012-07-30  Muhammad Bilal  <mbilal@codesorcery.com>
> 
>          PR gdb/15715
>          * top.c: Include "filenames.h".
>          (set_history_filename): New function.
>          (init_main): Install it as set hook of the "set history filename"
>          command.
> 
> /testsuit
> 2012-07-30  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

You've lost the period at end of this sentence.  It was there in
previous versions.  I'd like to be sure you learn to pay attention to
these details, hence the multiple iterations, but I'd like to avoid
having to go through them for all patches.   Please be attentive.  This
is OK with the missing period added (though as penance, I really should
have you go through one more round of posting before approving.  ;-) )

-- 
Pedro Alves

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

[Muhammad Bilal]

On 07/30/2013 04:00 PM, Pedro Alves wrote:
> On 07/30/2013 11:46 AM, Muhammad Bilal wrote:
>
>> 2012-07-30  Muhammad Bilal  <mbilal@codesorcery.com>
>>
>>           PR gdb/15715
>>           * top.c: Include "filenames.h".
>>           (set_history_filename): New function.
>>           (init_main): Install it as set hook of the "set history filename"
>>           command.
>>
>> /testsuit
>> 2012-07-30  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
> You've lost the period at end of this sentence.  It was there in
> previous versions.  I'd like to be sure you learn to pay attention to
> these details, hence the multiple iterations, but I'd like to avoid
> having to go through them for all patches.   Please be attentive.  This
> is OK with the missing period added (though as penance, I really should
> have you go through one more round of posting before approving.  ;-) )
>
committed

http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/top.c.diff?cvsroot=src&r1=1.239&r2=1.240
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/ChangeLog.diff?cvsroot=src&r1=1.15844&r2=1.15845
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/testsuite/gdb.base/setshow.exp.diff?cvsroot=src&r1=1.33&r2=1.34
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gdb/testsuite/ChangeLog.diff?cvsroot=src&r1=1.3748&r2=1.3749



Thanks,
-Bilal