From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29914 invoked by alias); 22 May 2013 17:51:35 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 29855 invoked by uid 89); 22 May 2013 17:51:34 -0000 X-Spam-SWARE-Status: No, score=-8.2 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 22 May 2013 17:51:34 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r4MHpVEH020095 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 22 May 2013 13:51:31 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r4MHpT1Z022951; Wed, 22 May 2013 13:51:30 -0400 Message-ID: <519D05A1.8030509@redhat.com> Date: Wed, 22 May 2013 17:51:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: mbilal CC: gdb-patches@sourceware.org, jan.kratochvil@redhat.com Subject: Re: [PATCH 1/7] PR gdb/15224 "set history filename" to by immediately converted to absolute path References: <51877A32.1030503@codesourcery.com> <51877A99.4060503@codesourcery.com> <5188AA15.5010904@codesourcery.com> <5188F70A.1030908@codesourcery.com> <518A0B2E.7000706@codesourcery.com> <519366E2.90105@codesourcery.com> <5193676B.7000008@codesourcery.com> In-Reply-To: <5193676B.7000008@codesourcery.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-05/txt/msg00852.txt.bz2 On 05/15/2013 11:46 AM, mbilal wrote: > On Wednesday, May 08, 2013 10:25 PM Pedro wrote: >> In addition to my previous comments, I realized that this will do the >>wrong thing with "set history filename ~/foo". Best use tilde_expand >>and gdb_realpath > > 'set history filename ~/foo' is working because following code is doing > same as you described . I have also attached test case for this. Ah, good. > 2013-05-15 Muhammad Bilal > > PR gdb/15224 It would have been better to file a different PR for this one, and then making 15224 depend on it. This is a preexisting bug, after all, that can be triggered no matter what the default history filename is. > * top.c (set_history_filename): New function. This is incomplete. You must have done something with the new function. :-) > 2013-05-15 Muhammad Bilal > > PR gdb/15224 > * gdb.base/setshow.exp: Test 'set history filename' relative > path. I think you meant "with a relative path". But this tests more now. Please make it complete. > diff --git a/gdb/top.c b/gdb/top.c > index 480b67e..45b83d7 100644 > --- a/gdb/top.c > +++ b/gdb/top.c > @@ -48,6 +48,7 @@ > #include "interps.h" > #include "observer.h" > #include "maint.h" > +#include "filenames.h" > > /* readline include files. */ > #include "readline/readline.h" > @@ -1693,6 +1694,17 @@ show_exec_done_display_p (struct ui_file *file, int from_tty, > value); > } > > +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 = concat (current_directory, "/", history_filename, > + (char *) NULL); > +} This leaks the previous history_filename. You can use reconcat instead to address that. > + > /* "set" command for the gdb_datadir configuration variable. */ > > static void > @@ -1777,7 +1789,7 @@ variable \"HISTSIZE\", or to 256 if this variable is not set."), > 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); > diff --git a/gdb/testsuite/gdb.base/setshow.exp b/gdb/testsuite/gdb.base/setshow.exp > index 6d250c0..5345b4b 100644 > --- a/gdb/testsuite/gdb.base/setshow.exp > +++ b/gdb/testsuite/gdb.base/setshow.exp > @@ -169,11 +169,24 @@ 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 > +gdb_test_multiple "show environment HOME" "show home directory" { > + -re "\nHOME = (.*).\n.*$gdb_prompt $" { > + set HOME $expect_out(1,string) > + } > +} > +#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 \"[file join $HOME foobar.baz]\"..*" \ > + "show history filename ([file join $HOME foobar.baz])" The gdb_test_multiple above only sets HOME on success. That means this will explode with an access to an unknown symbol ($HOME) if the gdb_test_multiple fails. You can put the result of [file join $HOME foobar.baz] in a variable and use that, instead of doing that twice: set filename [file join $HOME foobar.baz] Actually, "file join" uses the build system's directory separator, but we want the host's. Actually^2, GDB always uses '/' here, so we can ignore that and just do: set filename "$HOME/foobar.baz" Also, +gdb_test "show history filename" "The filename in which to record the command history is \"[file join $HOME foobar.baz]\"..*" \ + "show history filename ([file join $HOME foobar.baz])" Don't put that "([file join $HOME foobar.baz])" in the gdb.sum message, as that will make the message depend on system/whoever runs it. > #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 \"foobar.baz\"..*" "show history filename (foobar.baz)" > +gdb_test "show history filename" "The filename in which to record the command history is \"[file join [pwd] foobar.baz]\"..*" \ > + "show history filename ([file join [pwd] foobar.baz])" Another instance of the same problem mentioned before. [pwd] returns the current working directory on the build machine, not the host's. Use gdb's "pwd", not tcl's. As long as we're breaking the long line with a \, please do it before "The filename" too, so the line ends up a little shorter. > #test set history save on > gdb_test_no_output "set history save on" "set history save on" > #test show history save on > -- Pedro Alves